Re: "RETURNING PRIMARY KEY" syntax extension

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

.) Unnecessary new line + and - in the patch.
(src/backend/rewrite/rewriteManip.c::getInsertSelectQuery())
(src/include/rewrite/rewriteManip.h)

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

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

.) Already raised my concern regarding syntax limiting the current returning
clause implementation.

On Fri, Jun 27, 2014 at 6:22 AM, Ian Barwick <ian(at)2ndquadrant(dot)com> wrote:

>
>
> On 27/06/14 09:09, Tom Dunstan wrote:
>
> On 27 June 2014 06:14, Gavin Flower <GavinFlower(at)archidevsys(dot)co(dot)nz
>> <mailto:GavinFlower(at)archidevsys(dot)co(dot)nz>> wrote:
>>
>> On 27/06/14 00:12, Rushabh Lathia wrote:
>>
>> INSERT INTO dept VALUES (10,'ACCOUNTING','NEW YORK') returning
>> primary key, dname;
>>
>> I think allowing other columns with PRIMARY KEY would be more
>> useful syntax.
>> Even in later versions if we want to extend this syntax to return
>> UNIQUE KEY,
>> SEQUENCE VALUES, etc.. comma separation syntax will be more handy.
>>
>>
>> I agree 100%.
>>
>>
>> If the query is being hand-crafted, what's to stop the query writer from
>> just listing the
>>
> > id columns in the returning clause? And someone specifying RETURNING *
> is getting all the
> > columns anyway.
>
>>
>> The target use-case for this feature is a database driver that has just
>> done an insert and
>>
> > doesn't know what the primary key columns are - in that case mixing them
> with any other
> > columns is actually counter-productive as the driver won't know which
> columns are which.
> > What use cases are there where the writer of the query knows enough to
> write specific columns
> > in the RETURNING clause but not enough to know which column is the id
> column?
>
>>
>> Consistency is nice, and I can understand wanting to treat the PRIMARY
>> KEY bit as just
>> another set of columns in the list to return, but I'd hate to see this
>> feature put on
>>
> > the back-burner to support use-cases that are already handled by the
> current RETURNING
> > feature. Maybe it's easy to do, though.. I haven't looked into the
> implementation at all.
>
> Normal columns are injected into the query's returning list at parse time,
> whereas
> this version of the patch handles expansion of PRIMARY KEY at the rewrite
> stage, which
> would make handling a mix of PRIMARY KEY and normal output expressions
> somewhat tricky
> to handle. (In order to maintain the columns in their expected position
> you'd
> have to add some sort of placeholder/dummy TargetEntry to the returning
> list at parse
> time, then rewrite it later with the expanded primary key columns, or
> something
> equally messy).
>
> On the other hand, it should be fairly straightforward to handle a list of
> keywords
> for expansion (e.g. "RETURNING PRIMARY KEY, UNIQUE KEYS, SEQUENCE VALUES")
> should
> the need arise.
>
>
>
> Regards
>
> Ian Barwick
>
> --
> Ian Barwick http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

--
Rushabh Lathia