Tom Lane írta:
Zoltan Boszormenyi <[EMAIL PROTECTED]> 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 columnThis 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 behaviorback 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/
psql-serial-38.diff.gz
Description: Unix tar archive
---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
