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