Re: IDENTITY/GENERATED v36 Re: Final version of IDENTITY/GENERATED patch - Mailing list pgsql-patches
From | Zoltan Boszormenyi |
---|---|
Subject | Re: IDENTITY/GENERATED v36 Re: Final version of IDENTITY/GENERATED patch |
Date | |
Msg-id | 46139DC2.4010808@cybertec.at Whole thread Raw |
In response to | Re: IDENTITY/GENERATED v36 Re: Final version of IDENTITY/GENERATED patch (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: IDENTITY/GENERATED v36 Re: Final version of IDENTITY/GENERATED
patch
(Andrew Dunstan <andrew@dunslane.net>)
Re: IDENTITY/GENERATED v36 Re: Final version of IDENTITY/GENERATED patch (Tom Lane <tgl@sss.pgh.pa.us>) |
List | pgsql-patches |
Tom Lane írta: > Zoltan Boszormenyi <zboszor@dunaweb.hu> writes: > >> [ IDENTITY/GENERATED patch ] >> > > I got around to reviewing this today. > Thanks for the review. Sorry for the bit late reply, I was ill and then occupied with some other work. >> - unique index checks are done in two steps >> to avoid inflating the sequence if a unique index check >> is failed that doesn't reference the IDENTITY column >> > > This is just not acceptable --- there is nothing in the standard that > requires such behavior, But also there is nothing that would say not to do it. :-) And this way, there is be nothing that would separate IDENTITY from regular SERIALs only the slightly later value generation. The behaviour I proposed would be a big usability plus over the standard with less possible skipped values. > and I dislike the wide-ranging kluges you > introduced to support it. Can you see any other way to avoid skipping sequence values as much as possible? > Please get rid of that and put the behavior > back into ordinary DEFAULT-substitution where it belongs. You mean put the IDENTITY generation into rewriteTargetList()? And what about the "action at a distance" behaviour you praised so much before? (Which made the less-skipping behaviour implementable...) Anyway, I put it back. But it brought the consequence that GENERATED fields may reference IDENTITY columns, too, so I removed this limitation as well. >> - to minimize runtime impact of checking whether >> an index references the IDENTITY column and skipping it >> in the first step in ExecInsertIndexTuples(), I introduced >> a new attribute in the pg_index catalog. >> > > This is likewise unreasonably complex and fragile ... but it > goes away anyway if you remove the above, no? > Yes. > The patch appears to believe that OVERRIDING SYSTEM VALUE should be > restricted to the table owner, but I don't actually see any support > for that in the SQL2003 spec ... where did you get that from? > Somehow it felt wrong to allow everybody to use it. Limit removed. > I'm pretty dubious about the kluges in aclchk.c to automatically > grant/revoke on dependent sequences --- particularly the "revoke" > part. The problem with that is that it breaks if the same sequence > is being used to feed multiple tables. > OK, removed. > User-facing errors need to be ereport() not elog() so that they > can be translated and have appropriate SQLSTATEs reported. > elog is only for internal errors. > OK, changed. > One other thought is that the field names based on force_default > seemed confusing. I'd suggest that maybe "generated" would be > a better name choice. > I modified the names. force_default -> is_identity, attforceddef -> attgenerated I also fixed COPY without OVERRIDING SYSTEM VALUE regarding IDENTITY and GENERATED fields and modified the docs and the testcase according to your requested modifications. > Please fix and resubmit. > regards, tom lane > Thanks again for the review. Here's the new version with the modifications you requested. -- ---------------------------------- Zoltán Böszörményi Cybertec Geschwinde & Schönig GmbH http://www.postgresql.at/
Attachment
pgsql-patches by date: