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

Tom Lane írta:
> Zoltan Boszormenyi <zboszor(at)dunaweb(dot)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/