PostgreSQL Adapter Project - Code Review 4 Changes

From CDOT Wiki
Revision as of 15:06, 20 September 2011 by Gbatumbya (talk | contribs)
Jump to: navigation, search

Classes

Table

Task Status
getQuotedName
  • functionality is not needed just use SQLSchemamanager.quote() the way this is done in other sql manager code
 DONE

SQLAdapter

Task Status
setQueryTimeout()
  • remove the condition
  • it should be possible to set 0 timeout
 DONE
CancelTask -> StatementCancelationTask;
  • make it a top level class
 DONE

SQLSchemaManager

Task Status
addColumn
  • change the comment to "Reads a column from a JDBC driver result set and adds it to a table object".
  • Move rs argument to the last position
 DONE
The message about ignoring an index has changed
  • I think the extra info in the original message that the index was ignored was important.
 Diff is malfunctioning
There are extensive changes to readSchema?
  • What is the reason for these (or is it the diff malfunctioning)?
 Diff is malfunctioning
getAlterColumnToken
  • remove the separator flag.
  • Adding separators should be done in different code. Each method should do as little as possible.
 DONE
isPortable()
  • null/skipped columns should not affect portability. It is better to handle null columns in the caller.
  • (Methods should not try to do several tasks at once, in this case skipping invalid entries and determining portability.

This hampers method reusability.).

DONE
isWindowsCompatiable -> isWindowsCompatible DONE

StatementProxy/PreparedStatementProxy

Task Status
*Proxy.java -> *Wrapper.java
  • (this is not the proxy pattern, but more like the decorator pattern)
 DONE

PostgreSQLAdapter

Task Status
MAX_FIELD_PRECISION - is this from the PostgreSQL documentation?
  • The comment is incorrect. This is not 1048576*8*10 and is not 1GB, but 10MB. Remove "~~"
  • MAX_LO_SIZE - Remove "~~" from the comment. This is 2GB-1.
 DONE
BIND_BLOB
  • Why is this code needed? Under what circumstances
  • m_connection -> use getConnection instead.
  • nLO -> l

You already have this value, no need to get it again later on. Use Primitive.createLong() to create the long value.

  • If an error occurs, lo will not be closed. Please fix this.
  • setAutoCommit - why is this modified at all? We cannot simply change the autocommit flag on a connection.
 DONE
  • Remove BIND_BLOB
BIND_STRING, BIND_BINARY
  • you can optimize these with much simpler conditions than the ones in isLOB.
RequestTimeout should take into account non-positive timeouts. values <=0 do not get to requestTimeout
The timeout management functionality requestTimeout and releaseTimeout(rename to schedule and cancel)
  • should be moved to the cancelation task, as static methods.
  • The timer singleton instance (lazy initialized) should be there as well.
 DONE
unwrap
  • why is a pooled connection ever used there?
  • When could an exception occur?
  • Are we using the correct data source class?

- PGXADataSource is the one that gives pooled connections
- When using data load/schema tool
- Yes, PGXADataSource is the correct data source class

s_PGXAConnection, s_getQueryExecutor
  • what is the purpose of the conditional logic involving these?

-s_PGXAConnection used to ensure that reflected methods and properties loaded successfully
-s_getQueryExecutor used to ensure that unwrapped connection is a PGConnection

appendIdentitySuffix
  • Add a space after ";" for readability
 DONE
appendLiteral
  • TIMESTAMP_ORDINAL - this should be in UTC, I think that simply converting the timestamp to a string might not do it.
  • Try to see if using Primitive.toString() will provide the right format.
 DONE
  • used StringUtil.appendUTC
appendTypeConversion
  • TIMESTAMP_ORDINAL - please verify that this actually formats/parses UTC timestamps.
 DONE
  • modified postgresql setup scripts to set timezone to UTC
  • changed to use date_part('epoch', timestamp)
indexNameMatches
  • Spaces around commas are needed. Wrap the line after about 110 chars.
 DONE
isUnicode
  • Use lower case SQL keywords
  • Use bind variables - this is what makes prepared statements cacheable by the DB
  • Object [] { -> no spaces here
  • catch(SQLException e) space before (
  • Do not repackage the SQLException as a runtime exception - the method already allows throwing an SQLException
 DONE

PostgreSQLPreparedStatementProxy

Task Status
*Provide a class comment
  • Prepend a comment "// inner classes"
  • Use visibility declarators on members ("protected")
  • Fold lines after about 110 chars
 DONE
requestSavepoint
  • Do we need to generate unique names for save points? (E.g. can savepoint names on different connections conflict)?
  • Can the JDBC driver be used to manage savepoints?
  • Should we use prepared statements here?
  • The statement cleanup should be fixed. Right now it will not close the statement if an error occurs.
  • I think not, since the savepoints are only created inside a transaction block
  • No, the driver does not allow setSavePoint on XA transaction (org.postgresql.xa.PGXAConnection)
  • DONE

PostgreSQLSchemaManager

Task Status
appendColumnAlteration
  • "extract( epoch from" - remove space after (
  • Use getColumnAlterationToken
  • Are booleans indexable in Postgres? If not, we should use integers instead.
 DONE
  • Yes, it supports indexing booleans. Suggested use is with partial indexes on the condition that queries will use most often.
appendLOManagerTrigger
  • Method JavaDoc comments are required.
  • What does this do?
 DONE
appendTSIncrement
  • interval' – perhaps a space before "'" for readability?
 DONE
getGUIDExpr
  • Remove "XXX: from the comment
  • Move the comment to the class header comment
 DONE
isImplicitConversion
  • srcType == Primitive.BOOLEAN ) - remove space before )
 DONE
isValidColumnName
  • Spaces around "-"
 DONE
addColumn
  • This might be worth refactoring further by adding a template method in order to avoid code duplication with the base class.
 DONE
{

} undo the change to ;

DONE
Retrieved from "https://wiki.cdot.senecapolytechnic.ca/w/index.php?title=PostgreSQL_Adapter_Project_-_Code_Review_4_Changes&oldid=69550"