Re: "RETURNING PRIMARY KEY" syntax extension

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