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.	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 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.