Tom Lane írta:
Zoltan Boszormenyi <[EMAIL PROTECTED]> writes:

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?


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

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

Attachment: psql-serial-38.diff.gz
Description: Unix tar archive

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?


Reply via email to