PostgreSQL Adapter Project - Code Review 4 Changes

General

Task	Status
{ } undo the change to ;

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
I think the portable flag should not take into account columns that are not added to the table (return null in this case)?	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.
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.
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.
unwrap why is a pooled connection ever used there? When could an exception occur? Are we using the correct data source class?
s_PGXAConnection, s_getQueryExecutor what is the purpose of the conditional logic involving these?
appendIdentitySuffix Add a space after ";" for readability
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.
appendTypeConversion TIMESTAMP_ORDINAL - please verify that this actually formats/parses UTC timestamps.
indexNameMatches Spaces around commas are needed. Wrap the line after about 110 chars.
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

PostgreSQLPreparedStatementProxy

Task	Status
Provide a class comment
Prepend a comment "// inner classes"
Use visibility declarators on members ("protected")
Fold lines after about 110 chars
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.

PostgreSQLSchemaManager

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