SEO

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------


Zoltan Boszormenyi wrote:
> Hi,
>
> I think now this is really the final version.
>
> Changes in this version is:
> - when dropping a column that's referenced
> by a GENERATED column, the GENERATED
> column has to be also dropped. It's required by SQL:2003.
> - COPY table FROM works correctly with IDENTITY
> and GENERATED columns
> - extended testcase to show the above two
>
> To reiterate all the features that accumulated
> over time, here's the list:
>
> - extended catalog (pg_attribute) to keep track whether
> the column is IDENTITY or GENERATED
> - working GENERATED column that may reference
> other regular columns; it extends the DEFAULT
> infrastructure to allow storing complex expressions;
> syntax for such columns:
> colname type GENERATED ALWAYS AS ( expression )
> - working IDENTITY column whose value is generated
> after all other columns (regular or GENERATED)
> are assigned with values and validated via their
> NOT NULL and CHECK constraints; this allows
> tighter numbering - the only case when there may be
> missing serials are when UNIQUE indexes are failed
> (which is checked on heap_insert() and heap_update()
> and is a tougher nut to crack)
> syntax is:
> colname type GENERATED { ALWAYS | BY DEFAULT }
> AS IDENTITY [ ( sequence options ) ]
> the original SERIAL pseudo-type is left unmodified, the IDENTITY
> concept is new and extends on it - PostgreSQL may have multiple
> SERIAL columns in a table, but SQL:2003 requires that at most
> one IDENITY column may exist in a table at any time
> - Implemented the following TODOs:
> - %Have ALTER TABLE RENAME rename SERIAL sequence names
> - Allow SERIAL sequences to inherit permissions from the base table?
> Actually the roles that have INSERT or UPDATE permissions
> on the table gain permission on the sequence, too.
> This makes the following TODO unneeded:
> - Add DEFAULT .. AS OWNER so permission checks are done as the table owner
> This would be useful for SERIAL nextval() calls and CHECK constraints.
> - DROP DEFAULT is prohibited on GENERATED and IDENTITY columns
> - One SERIAL column can be upgraded to IDENTITY via
> ALTER COLUMN column SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
> Same for downgrading, via:
> ALTER COLUMN column DROP IDENTITY
> - COPY and INSERT may use OVERRIDING SYSTEM VALUE
> clause to override automatic generation and allow
> to import dumped data unmodified
> - Update is forbidden for GENERATED ALWAYS AS IDENTITY
> columns entirely and for GENERATED ALWAYS AS (expr)
> columns for other values than DEFAULT.
> - ALTER COLUMN SET <sequence options> for
> altering the supporting sequence; works on any
> SERIAL-like or IDENTITY columns
> - ALTER COLUMN RESTART [WITH] N
> for changing only the next generated number in the
> sequence.
> - The essence of pg_get_serial_sequence() is exported
> as get_relid_att_serial_sequence() to be used internally
> by checks.
> - CHECK constraints cannot reference IDENTITY or
> GENERATED columns
> - GENERATED columns cannot reference IDENTITY or
> GENERATED columns
> - dropping a column that's referenced by a GENERATED column
> also drops the GENERATED column
> - pg_dump dumps correct schema for IDENTITY and
> GENERATED columns:
> - ALTER COLUMN SET GENERATED ... AS IDENTITY
> for IDENTITY columns after ALTER SEQUENCE OWNED BY
> - correct GENERATED AS ( expression ) caluse in the table schema
> - pg_dump dumps COPY OVERRIDING SYSTEM VALUE
> for tables' date that have any GENERATED or
> GENERATED ALWAYS AS IDENTITY columns.
> - documentation and testcases
>
> Please, review.
>
> Best regards,
> Zolt?n B?sz?rm?nyi
>

[ application/x-tar is not supported, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Hi!

Thanks.

However, in the meantime I made some changes
so the IDENTITY column only advances its sequence
if it fails its CHECK constraints or UNIQUE indexes.
I still have some work with expression indexes.
Should I post an incremental patch against this version
or a full patch when it's ready?
An incremental patch can still be posted when the feature
is agreed to be in 8.3 and actually applied. It only changes
some details in the new feature and doesn't change
behaviour of existing features.

Best regards,
Zoltán Böszörményi

Bruce Momjian írta:
> Your patch has been added to the PostgreSQL unapplied patches list at:
>
> http://momjian.postgresql.org/cgi-bin/pgpatches
>
> It will be applied as soon as one of the PostgreSQL committers reviews
> and approves it.
>
> ---------------------------------------------------------------------------
>
>
> Zoltan Boszormenyi wrote:
>
>> Hi,
>>
>> I think now this is really the final version.
>>
>> Changes in this version is:
>> - when dropping a column that's referenced
>> by a GENERATED column, the GENERATED
>> column has to be also dropped. It's required by SQL:2003.
>> - COPY table FROM works correctly with IDENTITY
>> and GENERATED columns
>> - extended testcase to show the above two
>>
>> To reiterate all the features that accumulated
>> over time, here's the list:
>>
>> - extended catalog (pg_attribute) to keep track whether
>> the column is IDENTITY or GENERATED
>> - working GENERATED column that may reference
>> other regular columns; it extends the DEFAULT
>> infrastructure to allow storing complex expressions;
>> syntax for such columns:
>> colname type GENERATED ALWAYS AS ( expression )
>> - working IDENTITY column whose value is generated
>> after all other columns (regular or GENERATED)
>> are assigned with values and validated via their
>> NOT NULL and CHECK constraints; this allows
>> tighter numbering - the only case when there may be
>> missing serials are when UNIQUE indexes are failed
>> (which is checked on heap_insert() and heap_update()
>> and is a tougher nut to crack)
>> syntax is:
>> colname type GENERATED { ALWAYS | BY DEFAULT }
>> AS IDENTITY [ ( sequence options ) ]
>> the original SERIAL pseudo-type is left unmodified, the IDENTITY
>> concept is new and extends on it - PostgreSQL may have multiple
>> SERIAL columns in a table, but SQL:2003 requires that at most
>> one IDENITY column may exist in a table at any time
>> - Implemented the following TODOs:
>> - %Have ALTER TABLE RENAME rename SERIAL sequence names
>> - Allow SERIAL sequences to inherit permissions from the base table?
>> Actually the roles that have INSERT or UPDATE permissions
>> on the table gain permission on the sequence, too.
>> This makes the following TODO unneeded:
>> - Add DEFAULT .. AS OWNER so permission checks are done as the table owner
>> This would be useful for SERIAL nextval() calls and CHECK constraints.
>> - DROP DEFAULT is prohibited on GENERATED and IDENTITY columns
>> - One SERIAL column can be upgraded to IDENTITY via
>> ALTER COLUMN column SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
>> Same for downgrading, via:
>> ALTER COLUMN column DROP IDENTITY
>> - COPY and INSERT may use OVERRIDING SYSTEM VALUE
>> clause to override automatic generation and allow
>> to import dumped data unmodified
>> - Update is forbidden for GENERATED ALWAYS AS IDENTITY
>> columns entirely and for GENERATED ALWAYS AS (expr)
>> columns for other values than DEFAULT.
>> - ALTER COLUMN SET <sequence options> for
>> altering the supporting sequence; works on any
>> SERIAL-like or IDENTITY columns
>> - ALTER COLUMN RESTART [WITH] N
>> for changing only the next generated number in the
>> sequence.
>> - The essence of pg_get_serial_sequence() is exported
>> as get_relid_att_serial_sequence() to be used internally
>> by checks.
>> - CHECK constraints cannot reference IDENTITY or
>> GENERATED columns
>> - GENERATED columns cannot reference IDENTITY or
>> GENERATED columns
>> - dropping a column that's referenced by a GENERATED column
>> also drops the GENERATED column
>> - pg_dump dumps correct schema for IDENTITY and
>> GENERATED columns:
>> - ALTER COLUMN SET GENERATED ... AS IDENTITY
>> for IDENTITY columns after ALTER SEQUENCE OWNED BY
>> - correct GENERATED AS ( expression ) caluse in the table schema
>> - pg_dump dumps COPY OVERRIDING SYSTEM VALUE
>> for tables' date that have any GENERATED or
>> GENERATED ALWAYS AS IDENTITY columns.
>> - documentation and testcases
>>
>> Please, review.
>>
>> Best regards,
>> Zolt?n B?sz?rm?nyi
>>
>>
>
> [ application/x-tar is not supported, skipping... ]
>
>
>> ---------------------------(end of broadcast)---------------------------
>> TIP 4: Have you searched our list archives?
>>
>> http://archives.postgresql.org
>>
>
>


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Zoltan Boszormenyi wrote:
> Hi!
>
> Thanks.
>
> However, in the meantime I made some changes
> so the IDENTITY column only advances its sequence
> if it fails its CHECK constraints or UNIQUE indexes.
> I still have some work with expression indexes.
> Should I post an incremental patch against this version
> or a full patch when it's ready?

Full patch.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Zoltan Boszormenyi <zboszor@dunaweb.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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Zoltan Boszormenyi wrote:
> Tom Lane írta:
>
>>> - 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?



This doesn't strike me as a sensible design goal. Why not just live with
skipped values?

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Andrew Dunstan írta:
> Zoltan Boszormenyi wrote:
>> Tom Lane írta:
>>
>>>> - 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?
>
>
>
> This doesn't strike me as a sensible design goal. Why not just live
> with skipped values?
>
> cheers
>
> andrew

The idea wouldn't have considered to cross my mind
if Tom didn't mention the action-at-a-distance behaviour.
If you look back in the archives, my first working
IDENTITY was nothing more than the syntactic sugar
over the regular SERIAL.

--
----------------------------------
Zoltán Böszörményi
Cybertec Geschwinde & Schönig GmbH
http://www.postgresql.at/


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Zoltan Boszormenyi <zb@cybertec.at> writes:
> The idea wouldn't have considered to cross my mind
> if Tom didn't mention the action-at-a-distance behaviour.

AFAIR, that remark had to do with NEXT VALUE FOR, which SQL2003 defines
in a very weird way (it's not equivalent to nextval() as one would wish).
I don't see anything strange in the spec for GENERATED.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

Tom Lane írta:
> Zoltan Boszormenyi <zb@cybertec.at> writes:
>
>> The idea wouldn't have considered to cross my mind
>> if Tom didn't mention the action-at-a-distance behaviour.
>>
>
> AFAIR, that remark had to do with NEXT VALUE FOR, which SQL2003 defines
> in a very weird way (it's not equivalent to nextval() as one would wish).
> I don't see anything strange in the spec for GENERATED.
>
> regards, tom lane
>

Thanks for clarifying.
Please review the version I sent you.

--
----------------------------------
Zoltán Böszörményi
Cybertec Geschwinde & Schönig GmbH
http://www.postgresql.at/


---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

Zoltan Boszormenyi <zb@cybertec.at> writes:
> Here's the new version with the modifications you requested.

I see another problem with this patch: the code added to
ATExecDropColumn is a crude hack. It doesn't work anyway since this is
not the only possible way for columns to be dropped (another one that
comes to mind immediately is DROP TYPE ... CASCADE). The only correct
way to handle things is to let the dependency mechanism do it. I think
you would get the behavior you want if you make the generated columns
have AUTO rather than NORMAL dependencies on the columns they reference.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

I wrote:
> I see another problem with this patch: the code added to
> ATExecDropColumn is a crude hack. It doesn't work anyway since this is
> not the only possible way for columns to be dropped (another one that
> comes to mind immediately is DROP TYPE ... CASCADE). The only correct
> way to handle things is to let the dependency mechanism do it.

Actually, the whole question of dependencies for generated columns
probably needs some thought. What should happen if a function or
operator used in a GENERATED expression gets dropped? The result
for a normal column's default expression is that the default expression
goes away, but the column is still there. I suspect we don't want
that for a GENERATED column --- it would then be effectively a plain
column.

Along the same lines, is ALTER COLUMN DROP DEFAULT a legal operation
on a generated column? What about just replacing the expression with
ALTER COLUMN SET DEFAULT?

Before you get too excited about making generated columns disappear
automatically in all these cases, consider that dropping a column
is not something to be done lightly --- it might contain irreplaceable
data.

On second thought maybe the right approach is just to allow the default
expression to be dropped the same as it would be for an ordinary column,
and to make sure that if a GENERATED column doesn't (currently) have a
default, it is treated the same as an ordinary column.

This leads to the further thought that maybe GENERATED is not a property
of a column at all, but of its default (ie, it should be stored in
pg_attrdef not pg_attribute, which would certainly make the patch less
messy). AFAICS plain GENERATED merely indicates that the default
expression can depend on other columns, which is certainly a property
of the default --- you could imagine ALTER COLUMN SET DEFAULT GENERATED
AS ... to make a formerly plain column into a GENERATED one. I'm not
entirely sure about ALWAYS though.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate