PostgreSQL Adapter Project - Code Review 4 Changes

From CDOT Wiki
Revision as of 22:42, 14 September 2011 by Gbatumbya (talk | contribs) (→‎PostgreSQLAdapter)
Jump to: navigation, search

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
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.
Retrieved from "https://wiki.cdot.senecapolytechnic.ca/w/index.php?title=PostgreSQL_Adapter_Project_-_Code_Review_4_Changes&oldid=69255"