Re: IDENTITY/GENERATED v36 Re: Final version of IDENTITY/GENERATED patch

Zoltan Boszormenyi <zboszor(at)dunaweb(dot)hu> writes:
> [ IDENTITY/GENERATED patch ]

I got around to reviewing this today.

> - 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, and I dislike the wide-ranging kluges you
introduced to support it. Please get rid of that and put the behavior
back into ordinary DEFAULT-substitution where it belongs.

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

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?

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.

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.

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.

Please fix and resubmit.

regards, tom lane