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.

.) 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_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
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 <i...@2ndquadrant.com> wrote:

> On 27/06/14 09:09, Tom Dunstan wrote:
>  On 27 June 2014 06:14, Gavin Flower <gavinflo...@archidevsys.co.nz
>> <mailto:gavinflo...@archidevsys.co.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
>>         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
> should
> the need arise.
> Regards
> Ian Barwick
> --
>  Ian Barwick                   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services

Rushabh Lathia

Reply via email to