On 01/07/14 21:00, Rushabh Lathia wrote:
I spent some more time on the patch and here are my review comments.
.) Patch gets applied through patch -p1 (git apply fails)
.) trailing whitespace in the patch at various places
Not sure where you see this, unless it's in the tests, where it's
required.
.) Unnecessary new line + and - in the patch.
(src/backend/rewrite/rewriteManip.c::getInsertSelectQuery())
(src/include/rewrite/rewriteManip.h)
Fixed.
.) patch has proper test coverage and regression running cleanly.
.) In map_primary_key_to_list() patch using INDEX_ATTR_BITMAP_IDENTITY_KEY
bitmap to get the keycols. In IndexAttrBitmapKind there is also
INDEX_ATTR_BITMAP_KEY, so was confuse between INDEX_ATTR_BITMAP_KEY and
INDEX_ATTR_BITMAP_IDENTITY_KEY and also haven't found related comments in
the code. Later with use of testcase and debugging found confirmed that
INDEX_ATTR_BITMAP_IDENTITY_KEY only returns the Primary key.
Revised patch version (see other mail) fixes this by introducing
INDEX_ATTR_BITMAP_PRIMARY_KEY.
.) At present in patch when RETURNING PRIMARY KEY is used on table having no
primary key it throw an error. If I am not wrong JDBC will be using that into
getGeneratedKeys(). Currently this feature is implemented in the JDBC driver by
appending "RETURNING *" to the supplied statement. With this implementation
it will replace "RETURNING *" with "RETURNING PRIMARY KEY", right ? So just
wondering what JDBC expect getGeneratedKeys() to return when query don't
have primary key and user called executeUpdate() with
Statement.RETURN_GENERATED_KEYS? I looked at JDBC specification but its not
clear what it will return when table don't have keys. Can you please let us
know your finding on this ?
The spec [*] is indeed frustratingly vague:
The method Statement.getGeneratedKeys, which can be called to retrieve the
generated
value, returns a ResultSet object with a column for each automatically
generated value.
The methods execute, executeUpdate or Connection.prepareStatement accept an
optional
parameter, which can be used to indicate that any auto generated values
should be
returned when the statement is executed or prepared.
[*]
http://download.oracle.com/otn-pub/jcp/jdbc-4_1-mrel-spec/jdbc4.1-fr-spec.pdf
I understand this to mean that no rows will be returned if no auto-generated
values
are not present.
As-is, the patch will raise an error if the target table does not have a
primary key,
which makes sense from the point of view of the proposed syntax, but which will
make it impossible for the JDBC driver to implement the above understanding of
the spec
(i.e. return nothing if no primary key exists).
It would be simple enough not to raise an error in this case, but that means the
query would be effectively failing silently and I don't think that's desirable
behaviour.
A better solution would be to have an optional "IF EXISTS" clause:
RETURNING PRIMARY KEY [ IF EXISTS ]
which would be easy enough to implement.
Thoughts?
Ian Barwick
--
Ian Barwick http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
