Re: "RETURNING PRIMARY KEY" syntax extension

On Thu, Jul 3, 2014 at 6:56 AM, Ian Barwick <ian(at)2ndquadrant(dot)com> wrote:

> 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.
>
>
Please ignore the whitespace comment. Don't know somehow that was due to
my local changes in the branch.

>
> .) 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.
>
>
Looks good.

>
> .) 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).
>
> The basic reason for this patch is to allow JDBC to use this syntax for
getGeneratedKeys().
Now because this patch throw an error when table don't have any primary
key, this patch
won't be useful for the getGeneratedKeys() implementation.

> 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.
>
>
Yes, not to raise an error won't be desirable behavior.

> 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?
>
>
Hmm liked the idea about adding [ IF EXISTS ]. Looking at the grammar so
far we
having [ IF EXISTS ] option only for DDL commands (mainly DROP) not sure
whether
its ok to use such syntax for DML commands.

Others please share your thoughts/comments.

>
> Ian Barwick
>
> --
> Ian Barwick http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

--
Rushabh Lathia