Обсуждение: "RETURNING PRIMARY KEY" syntax extension

Hi,

The JDBC API provides the getGeneratedKeys() method as a way of retrieving
primary key values without the need to explicitly specify the primary key
column(s). This is a widely-used feature, however the implementation has significant
performance drawbacks.

Currently this feature is implemented in the JDBC driver by appending
"RETURNING *" to the supplied statement. However this means all columns of
affected rows will be returned to the client, which causes significant
performance problems, particularly on wide tables. To mitigate this, it would
be desirable to enable the JDBC driver to request only the primary key value(s).

One possible solution would be to have the driver request the primary key for
a table, but this could cause a race condition where the primary key could change,
and even if it does not, it would entail extra overhead.

A more elegant and universal solution, which would allow the JDBC driver to
request the primary key in a single request, would be to extend the RETURNING
clause syntax with the option PRIMARY KEY. This resolves during parse
analysis into the columns of the primary key, which can be done unambiguously
because the table is already locked by that point and the primary key cannot change.

A patch is attached which implements this, and will be added to the next commitfest.
A separate patch will be submitted to the JDBC project. Example usage shown below.


Regards

Ian Barwick

/* ---------------------------------------------- */
     postgres=# CREATE TABLE foo (id SERIAL PRIMARY KEY);
     CREATE TABLE

     postgres=# INSERT INTO foo VALUES(DEFAULT) RETURNING PRIMARY KEY;
      id
     ----
       1
     (1 row)

     INSERT 0 1

     postgres=# CREATE TABLE bar (id1 INT NOT NULL, id2 INT NOT NULL, PRIMARY KEY(id1, id2));
     CREATE TABLE
     postgres=# INSERT INTO bar VALUES(1,2) RETURNING PRIMARY KEY;
      id1 | id2
     -----+-----
        1 |   2
     (1 row)

     INSERT 0 1

     postgres=# INSERT INTO bar VALUES(2,1),(2,2) RETURNING PRIMARY KEY;
      id1 | id2
     -----+-----
        2 |   1
        2 |   2
     (2 rows)

     INSERT 0 2

     postgres=# CREATE TABLE no_pkey (id SERIAL NOT NULL);
     CREATE TABLE
     postgres=# INSERT INTO no_pkey VALUES(DEFAULT) RETURNING id;
      id
     ----
       1
     (1 row)

     INSERT 0 1
     postgres=# INSERT INTO no_pkey VALUES(DEFAULT) RETURNING PRIMARY KEY;
     ERROR:  Relation does not have any primary key(s)

/* ---------------------------------------------- */

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

Ian Barwick wrote
> Hi,
> 
> The JDBC API provides the getGeneratedKeys() method as a way of retrieving
> primary key values without the need to explicitly specify the primary key
> column(s). This is a widely-used feature, however the implementation has
> significant
> performance drawbacks.
> 
> Currently this feature is implemented in the JDBC driver by appending
> "RETURNING *" to the supplied statement. However this means all columns of
> affected rows will be returned to the client, which causes significant
> performance problems, particularly on wide tables. To mitigate this, it
> would
> be desirable to enable the JDBC driver to request only the primary key
> value(s).

Seems like a good idea.


>      ERROR:  Relation does not have any primary key(s)

"Relation does not have a primary key."
or
"Relation has no primary key." (preferred)

By definition it cannot have more than one so it must have none.

It could have multiple unique constraints but I do not believe they are
considered if not tagged as primary.

David J.





--
View this message in context:
http://postgresql.1045698.n5.nabble.com/RETURNING-PRIMARY-KEY-syntax-extension-tp5806462p5806463.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

David G Johnston wrote
> 
> Ian Barwick wrote
>> Hi,
>> 
>> The JDBC API provides the getGeneratedKeys() method as a way of
>> retrieving
>> primary key values without the need to explicitly specify the primary key
>> column(s). This is a widely-used feature, however the implementation has
>> significant
>> performance drawbacks.
>> 
>> Currently this feature is implemented in the JDBC driver by appending
>> "RETURNING *" to the supplied statement. However this means all columns
>> of
>> affected rows will be returned to the client, which causes significant
>> performance problems, particularly on wide tables. To mitigate this, it
>> would
>> be desirable to enable the JDBC driver to request only the primary key
>> value(s).
> Seems like a good idea.
>>      ERROR:  Relation does not have any primary key(s)
> "Relation does not have a primary key."
> or
> "Relation has no primary key." (preferred)
> 
> By definition it cannot have more than one so it must have none.
> 
> It could have multiple unique constraints but I do not believe they are
> considered if not tagged as primary.

Also,

I did see where you account for auto-updatable views but what about complex
views with instead of triggers?

These can still be the target of DML queries but are not guaranteed (though
can possibly) to return a well-defined primary key.  At worse an explicit
error about the view itself, not the apparent lack of primary key, should be
emitted.

David J.



--
View this message in context:
http://postgresql.1045698.n5.nabble.com/RETURNING-PRIMARY-KEY-syntax-extension-tp5806462p5806464.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On 09/06/14 14:47, David G Johnston wrote:
> Ian Barwick wrote
>> Hi,
>>
>> The JDBC API provides the getGeneratedKeys() method as a way of retrieving
>> primary key values without the need to explicitly specify the primary key
>> column(s). This is a widely-used feature, however the implementation has
>> significant
>> performance drawbacks.
>>
>> Currently this feature is implemented in the JDBC driver by appending
>> "RETURNING *" to the supplied statement. However this means all columns of
>> affected rows will be returned to the client, which causes significant
>> performance problems, particularly on wide tables. To mitigate this, it
>> would
>> be desirable to enable the JDBC driver to request only the primary key
>> value(s).
>
> Seems like a good idea.
>
>
>>       ERROR:  Relation does not have any primary key(s)
>
> "Relation does not have a primary key."
> or
> "Relation has no primary key." (preferred)
>
> By definition it cannot have more than one so it must have none.

Ah yes, amazing what a fresh pair of eyes does :). The plural is
the vestige of an earlier iteration which said something about
the relation not having any primary key column(s).

Will fix, thanks.

Regards

Ian Barwick


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

On 09/06/14 14:47, David G Johnston wrote:

Ian Barwick wrote

Hi,

The JDBC API provides the getGeneratedKeys() method as a way of retrieving
primary key values without the need to explicitly specify the primary key
column(s). This is a widely-used feature, however the implementation has
significant
performance drawbacks.

Currently this feature is implemented in the JDBC driver by appending
"RETURNING *" to the supplied statement. However this means all columns of
affected rows will be returned to the client, which causes significant
performance problems, particularly on wide tables. To mitigate this, it
would
be desirable to enable the JDBC driver to request only the primary key
value(s).

On 09/06/14 17:47, David G Johnston wrote:
> Ian Barwick wrote
>> Hi,
>>
>> The JDBC API provides the getGeneratedKeys() method as a way of retrieving
>> primary key values without the need to explicitly specify the primary key
>> column(s). This is a widely-used feature, however the implementation has
>> significant
>> performance drawbacks.
>>
>> Currently this feature is implemented in the JDBC driver by appending
>> "RETURNING *" to the supplied statement. However this means all columns of
>> affected rows will be returned to the client, which causes significant
>> performance problems, particularly on wide tables. To mitigate this, it
>> would
>> be desirable to enable the JDBC driver to request only the primary key
>> value(s).
> Seems like a good idea.
>
>
>>       ERROR:  Relation does not have any primary key(s)
> "Relation does not have a primary key."
> or
> "Relation has no primary key." (preferred)
>
> By definition it cannot have more than one so it must have none.
>
> It could have multiple unique constraints but I do not believe they are
> considered if not tagged as primary.
>
> David J.
>
>
>
>
>
> --
> View this message in context:
http://postgresql.1045698.n5.nabble.com/RETURNING-PRIMARY-KEY-syntax-extension-tp5806462p5806463.html
> Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
>
>From memory all unique keys can be considered 'candidate primary keys', 
but only one can be designated 'the PRIMARY KEY'.

I also like your preferred error message, and to the full extent of my 
decidedly Non-Authority, I hereby authorise it!  :-)


Cheers,
Gavin

On 06/09/2014 09:06 AM, Gavin Flower wrote:
> From memory all unique keys can be considered 'candidate primary keys',
> but only one can be designated 'the PRIMARY KEY'.

Almost.  Candidate keys are also NOT NULL.
-- 
Vik

David G Johnston <david.g.johnston@gmail.com> wrote:

>>       ERROR:  Relation does not have any primary key(s)
>
> "Relation does not have a primary key."
> or
> "Relation has no primary key." (preferred)

Project style says that the primary message should not capitalize
the first word, nor should it end in a period.  Detail and hints
should be in sentence style, but not the message itself.

http://www.postgresql.org/docs/9.3/interactive/error-style-guide.html#AEN100914

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/09/2014 06:58 AM, Ian Barwick wrote:
> Hi,
>
> The JDBC API provides the getGeneratedKeys() method as a way of
> retrieving
> primary key values without the need to explicitly specify the primary key
> column(s).
Is it defined by the standard, to return _only_ generated primary keys,
and not
for example generated alternate keys ?

Cheers

-- 
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

Ian Barwick <ian@2ndquadrant.com> writes:
> [ RETURNING PRIMARY KEY ]

It looks to me like this is coded to have the expansion of the "primary
key" done at parse time, which seems like fundamentally the wrong thing.
Consider a view or rule containing this clause; the pkey might be
different by the time execution rolls around.  It'd be better probably
if the rewriter or planner did the expansion (and threw the error for
no-primary-key, if necessary).

Alternatively, we could do it like this and consider that the view is
dependent on the primary key constraint, but that seems inflexible.

BTW, it seems like your representation of the clause was rather poorly
chosen: it forces changing a whole lot of code that otherwise would
not need to be changed.  I'd have left returningList alone and put the
returningPrimaryKey flag someplace else.
        regards, tom lane

On 09/06/14 23:42, Vik Fearing wrote:
> On 06/09/2014 09:06 AM, Gavin Flower wrote:
>>  From memory all unique keys can be considered 'candidate primary keys',
>> but only one can be designated 'the PRIMARY KEY'.
> Almost.  Candidate keys are also NOT NULL.
Yeah, obviously!
(Except, I did actually forget that - me bad.)

On 2014-06-09 13:42:22 +0200, Vik Fearing wrote:
> On 06/09/2014 09:06 AM, Gavin Flower wrote:
> > From memory all unique keys can be considered 'candidate primary keys',
> > but only one can be designated 'the PRIMARY KEY'.
> 
> Almost.  Candidate keys are also NOT NULL.

The list actually is a bit longer. They also cannot be partial.

There's generally also the restriction that for some contexts - like
e.g. foreign keys - primary/candidate keys may not be deferrable..

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 6/9/14, 8:35 AM, Hannu Krosing wrote:
> On 06/09/2014 06:58 AM, Ian Barwick wrote:
>> Hi,
>>
>> The JDBC API provides the getGeneratedKeys() method as a way of
>> retrieving
>> primary key values without the need to explicitly specify the primary key
>> column(s).
> Is it defined by the standard, to return _only_ generated primary keys,
> and not
> for example generated alternate keys ?

I was wondering that myself. I think it's certainly reasonable to expect someone would wan RETURNING SEQUENCE VALUES,
whichwould return the value of every column that owned a sequence (ie: ALTER SEQUENCE ... OWNED BY). ISTM that would
certainlyhandle the performance aspect of this, and it sounds more in line with what I'd expect getGeneratedKeys() to
do.
-- 
Jim C. Nasby, Data Architect                       jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net

A definite +1 on this feature. A while ago I got partway through hacking the hibernate postgres dialect to make it issue a RETURNING clause to spit out the primary key before I realised that the driver was already doing a RETURNING * internally.

On 10 June 2014 05:53, Jim Nasby <jim@nasby.net> wrote:

> I was wondering that myself. I think it's certainly reasonable to expect
> someone would wan RETURNING SEQUENCE VALUES, which would return the value of
> every column that owned a sequence (ie: ALTER SEQUENCE ... OWNED BY). ISTM
> that would certainly handle the performance aspect of this, and it sounds
> more in line with what I'd expect getGeneratedKeys() to do.

Keep in mind that not all generated keys come from sequences. Many people have custom key generator functions, including UUIDs and other exotic things like Instagram's setup [1].

RETURNING GENERATED KEYS perhaps, but then how do we determine that?

Any column that was filled with a default value? But that's potentially returning far more values than the user will want - I bet 99% of users just want their generated primary key.

The spec is a bit vague [2]: 

    Retrieves any auto-generated keys created as a result of executing

    this Statement object. If this Statement object did not generate any

    keys, an empty ResultSet object is returned.

    Note:If the columns which represent the auto-generated keys were

    not specified, the JDBC driver implementation will determine the

    columns which best represent the auto-generated keys.

The second paragraph refers to [3] and [4] where the application can specify which columns it's after. Given that there's a mechanism to specify which keys the application wants returned in the driver, and the driver in that case can just issue a RETURNING clause with a column list, my gut feel would be to just support returning primary keys as that will handle most cases of e.g. middleware like ORMs fetching that without needing to know the specific column names.

Cheers

Tom

[1] http://instagram-engineering.tumblr.com/post/10853187575/sharding-ids-at-instagram

[2] http://docs.oracle.com/javase/7/docs/api/java/sql/Statement.html#getGeneratedKeys()

[3] http://docs.oracle.com/javase/7/docs/api/java/sql/Statement.html#execute(java.lang.String,%20int[])

[4] http://docs.oracle.com/javase/7/docs/api/java/sql/Statement.html#execute(java.lang.String,%20java.lang.String[])

-- 
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 10 June 2014 17:49, Hannu Krosing <hannu@2ndquadrant.com> wrote:

RETURNING GENERATED KEYS perhaps, but then how do we determine that?

What about RETURNING CHANGED FIELDS ?

Might be quite complicated technically, but this is what is probably wanted.

Seems to be getting further away from something that describes the main use
case - changed fields sounds like something that would apply to an update statement.

Any column that was filled with a default value? But that's potentially returning far more values than the user will want - I bet 99% of users just want their generated primary key.

Probably not true - you would want your ORM model to be in sync with what is database after you save it if you plan to do any further processing using it.

Well, yes, but since RETURNING is non-standard most ORMs are unlikely to support fetching other generated values that way anyway. The ones that I've dealt with will do an insert, then a select to get the extra fields. I don't know if other JDBC drivers allow applications to just specify any old list of non-key columns to the execute method, but I suspect not, given that the way they fetch those columns is rather less general-purpose than our RETURNING syntax.

 

The second paragraph refers to [3] and [4] where the application can specify which columns it's after. Given that there's a mechanism to specify which keys the application wants returned in the driver, and the driver in that case can just issue a RETURNING clause with a column list, my gut feel would be to just support returning primary keys as that will handle most cases of e.g. middleware like ORMs fetching that without needing to know the specific column names.

Why not then just leave the whole thing as it is on server side, and let the ORM specify which "generated keys" it wants ?

Because java-based ORMs (at least) mostly don't have to - other server/driver combos manage to implement getGeneratedKeys() without being explicitly given a column list, they just do the sane thing and return the appropriate identity column or whatever for the inserted row.

 

I agree that in hand-crafted JDBC there's no particular problem in making a user specify a column list, (although I don't think I've EVER seen anyone actually do that in the wild), but most middleware will expect getGeneratedKeys() to just work and we should try to do something about making that case work a bit more efficiently than it does now.

Cheers

Tom

-- 
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 06/09/2014 07:13 PM, Andres Freund wrote:
> On 2014-06-09 13:42:22 +0200, Vik Fearing wrote:
>> On 06/09/2014 09:06 AM, Gavin Flower wrote:
>>> From memory all unique keys can be considered 'candidate primary keys',
>>> but only one can be designated 'the PRIMARY KEY'.
>>
>> Almost.  Candidate keys are also NOT NULL.
> 
> The list actually is a bit longer. They also cannot be partial.

What?  AFAIK, that only applies to an index.  How can the data itself be
partial?

> There's generally also the restriction that for some contexts - like
> e.g. foreign keys - primary/candidate keys may not be deferrable..

Again, what is deferrable data?
-- 
Vik

On 2014-06-11 00:21:58 +0200, Vik Fearing wrote:
> On 06/09/2014 07:13 PM, Andres Freund wrote:
> > On 2014-06-09 13:42:22 +0200, Vik Fearing wrote:
> >> On 06/09/2014 09:06 AM, Gavin Flower wrote:
> >>> From memory all unique keys can be considered 'candidate primary keys',
> >>> but only one can be designated 'the PRIMARY KEY'.
> >>
> >> Almost.  Candidate keys are also NOT NULL.
> > 
> > The list actually is a bit longer. They also cannot be partial.
> 
> What?  AFAIK, that only applies to an index.  How can the data itself be
> partial?

I don't follow? Gavin above talked about unique keys - which in postgres
you can create using CREATE UNIQUE INDEX. And if you make those partial
they can't be used for this purpose.

> > There's generally also the restriction that for some contexts - like
> > e.g. foreign keys - primary/candidate keys may not be deferrable..
> 
> Again, what is deferrable data?

You can define primary/unique constraints to be deferrable. c.f. CREATE
TABLE docs.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-06-11 00:21:58 +0200, Vik Fearing wrote:
>> What?  AFAIK, that only applies to an index.  How can the data itself be
>> partial?

> I don't follow? Gavin above talked about unique keys - which in postgres
> you can create using CREATE UNIQUE INDEX. And if you make those partial
> they can't be used for this purpose.

I have a feeling this conversation is going in the wrong direction.
ISTM that to be useful at all, the set of columns that would be returned
by a clause like this has to be *extremely* predictable; otherwise the
application won't know what to do with the results.  If the app has to
examine the table's metadata to identify what it's getting, what's the
point of the feature at all as opposed to just listing the columns you
want explicitly?  So I doubt that the use-case for anything more
complicated than returning the primary key, full stop, is really there.

I'm not even 100% sold that automatically returning the primary key
is going to save any application logic.  Could somebody point out
*exactly* where an app is going to save effort with this type of
syntax, compared to requesting the columns it wants by name?
Is it going to save enough to justify depending on a syntax that won't
be universal for a long time to come?
        regards, tom lane

On 14/06/12 18:46, Jochem van Dieten wrote:
> On Wed, Jun 11, 2014 at 2:39 AM, Tom Lane wrote:
> 
>     I'm not even 100% sold that automatically returning the primary key
>     is going to save any application logic.  Could somebody point out
>     *exactly* where an app is going to save effort with this type of
>     syntax, compared to requesting the columns it wants by name?
> 
> 
> I haven't checked the code, but I am hoping it will help with the problem 
> where a RETURNING * is added to a statement that is not an insert or update
> by the JDBC driver. That has been reported on the JDBC list at least twice,
> and the proposed workaround is neither very elegant nor very robust:
> https://groups.google.com/forum/#!msg/pgsql.interfaces.jdbc/7WY60JX3qyo/-v1fqDqLQKwJ

Unfortunately that seems to be a JDBC-specific issue, which is outside
of the scope of this particular patch (which proposes additional server-side
syntax intended to make RETURNING * operations more efficient for
certain use cases, but which is in itself not a JDBC change).


Regards

Ian Barwick

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

On 2014-06-12 13:58:31 +0200, Jochem van Dieten wrote:
> But the obvious way to fix the JDBC issue is not to fix it by adding a
> 'mini parser' on the JDBC side, but to make SELECT ... RETURNING PRIMARY
> KEY a regular select that silently ignores the returning clause and doesn't
> throw an error on the server-side.

Brr. Then it'd need to be added to all statements, not just SELECT. I've
my doubts that's going to happen.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 14/06/12 20:58, Jochem van Dieten wrote:
> On Thu, Jun 12, 2014 at 12:25 PM, Ian Barwick wrote:
> 
>     On 14/06/12 18:46, Jochem van Dieten wrote:
>     > I haven't checked the code, but I am hoping it will help with the problem
>     > where a RETURNING * is added to a statement that is not an insert or update
>     > by the JDBC driver. That has been reported on the JDBC list at least twice,
>     > and the proposed workaround is neither very elegant nor very robust:
>     > https://groups.google.com/forum/#!msg/pgsql.interfaces.jdbc/7WY60JX3qyo/-v1fqDqLQKwJ
> 
>     Unfortunately that seems to be a JDBC-specific issue, which is outside
>     of the scope of this particular patch (which proposes additional server-side
>     syntax intended to make RETURNING * operations more efficient for
>     certain use cases, but which is in itself not a JDBC change).
> 
> 
> But the obvious way to fix the JDBC issue is not to fix it by adding a 'mini parser' on
> the JDBC side, but to make SELECT ... RETURNING PRIMARY KEY a regular select that silently
> ignores the returning clause and doesn't throw an error on the server-side.
> 
> That might still be outside the scope of this particular patch, but it would provide 
> (additional) justification if it were supported.

That would be adding superfluous, unused and unusable syntax of no potential value
(there is no SELECT ... RETURNING and it wouldn't make any sense if there was) as a
workaround for a driver issue - not going to happen.

Regards

Ian Barwick


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

Hi

On 14/06/25 15:13, Rushabh Lathia wrote:
> Hello All,
> 
> I assigned my self as reviewer of the patch. I gone through the
> mail chain discussion and in that question has been raised about
> the feature and its implementation, so would like to know what is
> the current status of this project/patch.
> 
> Regards,

I'll be submitting a revised version of this patch very shortly.


Regards

Ian Barwick

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

On 25/06/14 16:04, Ian Barwick wrote:
> Hi
>
> On 14/06/25 15:13, Rushabh Lathia wrote:
>> Hello All,
>>
>> I assigned my self as reviewer of the patch. I gone through the
>> mail chain discussion and in that question has been raised about
>> the feature and its implementation, so would like to know what is
>> the current status of this project/patch.
>>
>> Regards,
>
> I'll be submitting a revised version of this patch very shortly.

Revised version of the patch attached, which implements the expansion
of "primary key" in the rewrite phase per Tom Lane's suggestion upthread [*]

[*] http://www.postgresql.org/message-id/28583.1402325169@sss.pgh.pa.us


Regards

Ian Barwick

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

On 27/06/14 00:12, Rushabh Lathia wrote:
> Thanks for sharing latest patch.
>
> For me this syntax is limiting the current returning clause 
> implementation.
> Because its not allowing the returning PRIMARY KEYS with other 
> columns, and
> if user or application require that they need to do RETURNING *. Following
> syntax not working, which i think is very useful in term if someone 
> want to use
> returning clause with PRIMARY KEY as well as some other table columns.
>
> 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.
>
> Others please share your inputs/thoughts.
>
>
[...]

I agree 100%.

I note that DELETE & INSERT have a RETURNING clause.  So the semantics 
and syntax should be the same, as much as is practicable.


Cheers,
Gavin

On 27/06/14 09:09, Tom Dunstan wrote:
> On 27 June 2014 06:14, Gavin Flower <GavinFlower@archidevsys.co.nz <mailto:GavinFlower@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
returningclause? 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
primarykey columns are - in that case mixing them with any other> columns is actually counter-productive as the driver
won'tknow which columns are which.> What use cases are there where the writer of the query knows enough to write
specificcolumns> 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-casesthat are already handled by the current RETURNING> feature. Maybe it's easy to do, though.. I haven't looked
intothe 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

On Tue, Jul 1, 2014 at 8:00 AM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> .) 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.

Actually, that depends on how REPLICA IDENTITY is set.  IOW, you can't
assume that will give you the primary key.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 14/07/01 23:13, Robert Haas wrote:
> On Tue, Jul 1, 2014 at 8:00 AM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>> .) 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.
> 
> Actually, that depends on how REPLICA IDENTITY is set.  IOW, you can't
> assume that will give you the primary key.

Damn, fooled by the name. Thanks for the info; I'll rework the patch
accordingly.

Regards


Ian Barwick


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

On 02/07/14 15:16, Ian Barwick wrote:
> On 14/07/01 23:13, Robert Haas wrote:
>> On Tue, Jul 1, 2014 at 8:00 AM, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>>> .) 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.
>>
>> Actually, that depends on how REPLICA IDENTITY is set.  IOW, you can't
>> assume that will give you the primary key.
>
> Damn, fooled by the name. Thanks for the info; I'll rework the patch
> accordingly.

Attached version implements an IndexAttrBitmapKind "INDEX_ATTR_BITMAP_PRIMARY_KEY",
which will return the primary key column(s). Note this would require a catalog
version bump.


Regards

Ian Barwick



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

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
objectwith a column for each automatically generated value.    The methods execute, executeUpdate or
Connection.prepareStatementaccept an optional    parameter, which can be used to indicate that any auto generated
valuesshould 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

Rushabh Lathia <rushabh.lathia@gmail.com> writes:
> On Thu, Jul 3, 2014 at 6:56 AM, Ian Barwick <ian@2ndquadrant.com> wrote:
>> A better solution would be to have an optional "IF EXISTS" clause:
>> RETURNING PRIMARY KEY [ IF EXISTS ]

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

TBH, I thought that RETURNING PRIMARY KEY was already fairly iffy
in that the application would have little idea what it was getting back.
IF EXISTS would make it so spongy as to be useless, IMO.

It sounds to me like designing this for JDBC's getGeneratedKeys method
is a mistake.  There isn't going to be any way that the driver can support
that without having looked at the table's metadata for itself, and if
it's going to do that then it doesn't need a crutch that only partially
solves the problem anyhow.
        regards, tom lane

On Thu, Jul 3, 2014 at 7:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> TBH, I thought that RETURNING PRIMARY KEY was already fairly iffy
> in that the application would have little idea what it was getting back.
> IF EXISTS would make it so spongy as to be useless, IMO.

Seems easy enough to look at the count of columns in the result set.
But it seems like noise words -- if you don't put IF EXISTS then
surely you'll get the same behaviour anyways, no?

Fwiw when I wrote ORM-like layers I used to describe the output of the
query, including sometimes issuing WHERE 1<>0 queries and looking at
the output column types when I needed that before executing the query.
Using table metadata would have required a much more in depth
understanding of how the database worked and also wouldn't handle
expressions, joins, set operations, etc.

-- 
greg

Greg Stark <stark@mit.edu> writes:
> On Thu, Jul 3, 2014 at 7:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> TBH, I thought that RETURNING PRIMARY KEY was already fairly iffy
>> in that the application would have little idea what it was getting back.
>> IF EXISTS would make it so spongy as to be useless, IMO.

> Seems easy enough to look at the count of columns in the result set.

Sure, if you know how many columns are in the table to begin with; but
that goes back to having table metadata.  If you don't, you're probably
going to be issuing "RETURNING *", and AFAICS "RETURNING *, PRIMARY KEY"
would be entirely useless (even without IF EXISTS :-().

I'm still unconvinced that there's a robust use-case here.
        regards, tom lane

Tom Dunstan <pgsql@tomd.cc> writes:
> On 4 July 2014 00:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It sounds to me like designing this for JDBC's getGeneratedKeys method
>> is a mistake.  There isn't going to be any way that the driver can support
>> that without having looked at the table's metadata for itself, and if
>> it's going to do that then it doesn't need a crutch that only partially
>> solves the problem anyhow.

> Sure it can - it append RETURNING PRIMARY KEY and hand back a ResultSet
> from whatever was returned. It's CURRENTLY doing that, but it's appending
> RETURNING * and leaving it up to the caller of getGeneratedKeys() to work
> out which columns the caller is interested in.

I might be mistaken, but as I read the spec for getGeneratedKeys, whether
or not a column is in a primary key has got nothing to do with whether
that column should be returned.  It looks to me like what you're supposed
to return is columns with computed default values, eg, serial columns.
(Whether we should define it as being *exactly* columns created by the
SERIAL macro is an interesting question, but for sure those ought to be
included.)  Now, the pkey might be a serial column ... but it equally
well might not be; it might not have any default expression at all.
And there certainly could be serial columns that weren't in the pkey.

The fact that the spec is kinda fuzzy doesn't entitle us to ignore the
plain meaning of the term "generated key".
        regards, tom lane

On Fri, Jul 4, 2014 at 12:36 AM, Rushabh Lathia
<rushabh.lathia@gmail.com> wrote:
> On Fri, Jul 4, 2014 at 7:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Tom Dunstan <pgsql@tomd.cc> writes:
>> > On 4 July 2014 00:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >> It sounds to me like designing this for JDBC's getGeneratedKeys method
>> >> is a mistake.  There isn't going to be any way that the driver can
>> >> support
>> >> that without having looked at the table's metadata for itself, and if
>> >> it's going to do that then it doesn't need a crutch that only partially
>> >> solves the problem anyhow.
>>
>> > Sure it can - it append RETURNING PRIMARY KEY and hand back a ResultSet
>> > from whatever was returned. It's CURRENTLY doing that, but it's
>> > appending
>> > RETURNING * and leaving it up to the caller of getGeneratedKeys() to
>> > work
>> > out which columns the caller is interested in.
>>
>> I might be mistaken, but as I read the spec for getGeneratedKeys, whether
>> or not a column is in a primary key has got nothing to do with whether
>> that column should be returned.  It looks to me like what you're supposed
>> to return is columns with computed default values, eg, serial columns.
>> (Whether we should define it as being *exactly* columns created by the
>> SERIAL macro is an interesting question, but for sure those ought to be
>> included.)  Now, the pkey might be a serial column ... but it equally
>> well might not be; it might not have any default expression at all.
>> And there certainly could be serial columns that weren't in the pkey.
>
> 100% agree with Tom.

Well, we could have a RETURNING GENERATED COLUMNS construct, or
something like that.  I can certainly see the usefulness of such a
thing.

As a general note not necessarily related to this specific issue, I
think we should be careful not to lose sight of the point of this
feature, which is to allow pgsql-jdbc to more closely adhere to the
JDBC specification.  While it's great if this feature has general
utility, if it doesn't help pgsql-jdbc, then it fails to achieve the
author's original intention.  We need to make sure we're not throwing
up too many obstacles in the way of better driver compliance if we
want to have good drivers.

As a code-level comment, I am not too find of adding yet another value
for IndexAttrBitmapKind; every values we compute in
RelationGetIndexAttrBitmap adds overhead for everyone, even people not
using the feature.  Admittedly, it's only paid once per relcache load,
but does
map_primary_key_to_list() really need this information cached, or can
it just look through the index list for the primary key, and then work
directly from that?

Also, map_primary_key_to_list() tacitly assumes that an auto-updatable
view has only 1 from-list item.  That seems like a good thing to check
with an Assert(), just in case we should support some other case in
the future.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company