Re: "RETURNING PRIMARY KEY" syntax extension

On Mon, Jul 7, 2014 at 5:50 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Fri, Jul 4, 2014 at 12:36 AM, Rushabh Lathia
> <rushabh(dot)lathia(at)gmail(dot)com> wrote:
> > On Fri, Jul 4, 2014 at 7:37 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Tom Dunstan <pgsql(at)tomd(dot)cc> writes:
> >> > On 4 July 2014 00:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
>
>
Another code-level comment is, why we need RowExclusiveLock before calling
map_primary_key_to_list() ? I think we should have explanation for that.

+ /* Handle RETURNING PRIMARY KEY */
+ if(query->returningPK)
+ {
+ RangeTblEntry *rte = (RangeTblEntry *)
list_nth(query->rtable, query->resultRelation - 1);
+ Relation rel = heap_open(rte->relid, RowExclusiveLock);
+ query->returningList = map_primary_key_to_list(rel,
query);
+ heap_close(rel, NoLock);
+ }

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

--
Rushabh Lathia