Re: "RETURNING PRIMARY KEY" syntax extension - Mailing list pgsql-hackers

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