Thread: Re: Final version of IDENTITY/GENERATED patch

Resending compressed, it seems pgsql-patches
doesn't let me post so large patches.

Zoltan Boszormenyi írta:
> Hi,
>
> I have finished my GENERATED/IDENTITY patch
> and now it does everything I wanted it to do. Changes
> from the previous version:
>
> - GENERATED columns work now
> - extended testcase to test some GENERATED expressions
> - extended documentation
>
> Now it comes in an uncompressed context diff form,
> out of habit I sent unified diffs before, sorry for that.
> It applies to current CVS.
>
> Please, review.
>
> Thanks in advance,
> Zoltán Böszörményi

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

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. +

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

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. +

Hi

Bruce Momjian írta:
> 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.
>

Then here it is. Now it's really finished, I promise. :-)
Changes:

- 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
- 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. I had to place it
  in the middle of the fixed size attributes because I had
  mysterious crashes otherwise. This means the attributes
  are renumbered. This attribute is determined during
  CREATE INDEX and recomputed for all indexes defined
  on the table during ALTER TABLE SET/DROP IDENTITY.
- as a consequence, IDENTITY/GENERATED can now
  have CHECK constraints, this limit was removed.
- modified testcase for the above changes
- reworded documentation

Please, review.

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

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
>
> Bruce Momjian ?rta:
> > 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.
> >
>
> Then here it is. Now it's really finished, I promise. :-)
> Changes:
>
> - 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
> - 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. I had to place it
>   in the middle of the fixed size attributes because I had
>   mysterious crashes otherwise. This means the attributes
>   are renumbered. This attribute is determined during
>   CREATE INDEX and recomputed for all indexes defined
>   on the table during ALTER TABLE SET/DROP IDENTITY.
> - as a consequence, IDENTITY/GENERATED can now
>   have CHECK constraints, this limit was removed.
> - modified testcase for the above changes
> - reworded documentation
>
> Please, review.
>
> Best regards,
> Zolt?n B?sz?rm?nyi
>

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

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
>        subscribe-nomail command to majordomo@postgresql.org so that your
>        message can get through to the mailing list cleanly

--
  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. +

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

Tom Lane írta:
> Zoltan Boszormenyi <zboszor@dunaweb.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/

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

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/

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

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/

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

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

Tom Lane írta:
> 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.
>>

I will try that.

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

No, I would want the DROP FUNCTION to be cancelled if used in
a GENERATED, but a DROP ... CASCADE would drop it, too.
So, DEPENDENCY_NORMAL will keep the referencing object
but DEPENDENCY_AUTO would drop it too if the referenced
object is dropped?

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

Neither of these options are legal for GENERATED columns,
AFAIK I prohibited them already.

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

The standard says that the GENERATED column should be
dropped silently if either of the referenced columns is dropped.
I haven't seen anything about the expression, though.

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

The standard says somewhere that GENERATED columns
can only be added to and dropped from a table.

My observation is: I deleted my hack from ATExecDropColumn()
and now I cannot drop the referenced column without CASCADE.
The comment in StoreAttrDefault() says the objects in the expression
will have dependencies registered. I guess "objects" also means functions?
This way, if I do explicit recordDependencyOn(, DEPENDENCY_AUTO)
on referenced columns then the standard requirement will
be satisfied, i.e. dropping columns will drop GENERATED columns
silently that reference the said column but . Am I right? Or how about using
recordDependencyOnSingleRelExpr(... , DEP_NORMAL, DEP_AUTO ) ?

>             regards, tom lane
>
>


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

Zoltan Boszormenyi <zb@cybertec.at> writes:
>> 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.

> The standard says that the GENERATED column should be
> dropped silently if either of the referenced columns is dropped.

[ itch... ]  I think a pretty good case could be made for ignoring that
provision, on the grounds that it's a foot-gun, and that it's not very
important to follow the standard slavishly on this point because it's
hard to conceive of any application actually relying on that behavior.

You could probably implement the auto-drop behavior with some combination
of (a) AUTO rather than NORMAL dependencies from the default expression
to the stuff it depends on and (b) INTERNAL rather than AUTO dependency
from the default expression to its column.  But I really question
whether this is a good idea.

> The standard says somewhere that GENERATED columns
> can only be added to and dropped from a table.

This argument carries no weight at all --- there is plenty of stuff in
PG that is an extension of the capabilities listed in the spec.

            regards, tom lane

Tom Lane írta:
> Zoltan Boszormenyi <zb@cybertec.at> writes:
>
>>> 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.
>>>
>
>
>> The standard says that the GENERATED column should be
>> dropped silently if either of the referenced columns is dropped.
>>
>
> [ itch... ]  I think a pretty good case could be made for ignoring that
> provision, on the grounds that it's a foot-gun, and that it's not very
> important to follow the standard slavishly on this point because it's
> hard to conceive of any application actually relying on that behavior.
>
> You could probably implement the auto-drop behavior with some combination
> of (a) AUTO rather than NORMAL dependencies from the default expression
> to the stuff it depends on and (b) INTERNAL rather than AUTO dependency
> from the default expression to its column.  But I really question
> whether this is a good idea.
>

So, all dependency should be NORMAL to require
manual CASCADE to avoid accidental data loss.

I have two questions about the dependency system.

1. Is there a built-in defense to avoid circular dependencies?

2. If I register dependencies between column, is there a way
    to retrieve all table/column type dependencies for a depender column?

What I would like to achieve is to lift the limit that
a GENERATED column cannot reference another one.
Only self-referencing should be disallowed.

>> The standard says somewhere that GENERATED columns
>> can only be added to and dropped from a table.
>>
>
> This argument carries no weight at all --- there is plenty of stuff in
> PG that is an extension of the capabilities listed in the spec.
>

Point taken. So, just like with SET / DROP IDENTITY,
I should implement SET GENERATED ALWAYS
and DROP GENERATED.

>             regards, tom lane
>
>


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

Zoltan Boszormenyi <zb@cybertec.at> writes:
> I have two questions about the dependency system.

> 1. Is there a built-in defense to avoid circular dependencies?

It doesn't have a problem with them, if that's what you mean.

> 2. If I register dependencies between column, is there a way
>     to retrieve all table/column type dependencies for a depender column?

You can scan pg_depend.

> What I would like to achieve is to lift the limit that
> a GENERATED column cannot reference another one.

I would counsel not doing that, mainly because then you will have to
solve an evaluation-order problem at runtime.

> Point taken. So, just like with SET / DROP IDENTITY,
> I should implement SET GENERATED ALWAYS
> and DROP GENERATED.

If you think of it as a property of the default expression, then DROP
DEFAULT covers both cases, you don't need DROP GENERATED...

            regards, tom lane

Tom Lane írta:
> Zoltan Boszormenyi <zb@cybertec.at> writes:
>
>> I have two questions about the dependency system.
>>
>
>
>> 1. Is there a built-in defense to avoid circular dependencies?
>>
>
> It doesn't have a problem with them, if that's what you mean.
>
>
>> 2. If I register dependencies between column, is there a way
>>     to retrieve all table/column type dependencies for a depender column?
>>
>
> You can scan pg_depend.
>
>
>> What I would like to achieve is to lift the limit that
>> a GENERATED column cannot reference another one.
>>
>
> I would counsel not doing that, mainly because then you will have to
> solve an evaluation-order problem at runtime.
>

OK.

>> Point taken. So, just like with SET / DROP IDENTITY,
>> I should implement SET GENERATED ALWAYS
>> and DROP GENERATED.
>>
>
> If you think of it as a property of the default expression, then DROP
> DEFAULT covers both cases, you don't need DROP GENERATED...
>

So, I should allow DROP DEFAULT, implement
SET DEFAULT GENERATED ALWAYS AS
and modify the catalog so the GENERATED property
is part of pg_attrdef. What about IDENTITY?
Should it also be part of pg_attrdef? There are two ways
to implement it: have or don't have a notion of it.
The latter would treat GENERATED BY DEFAULT AS IDENTITY
the same as SERIAL.

>             regards, tom lane
>

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

Zoltan Boszormenyi <zb@cybertec.at> writes:
> So, I should allow DROP DEFAULT, implement
> SET DEFAULT GENERATED ALWAYS AS
> and modify the catalog so the GENERATED property
> is part of pg_attrdef.

Sounds good.

> What about IDENTITY?
> Should it also be part of pg_attrdef? There are two ways
> to implement it: have or don't have a notion of it.
> The latter would treat GENERATED BY DEFAULT AS IDENTITY
> the same as SERIAL.

Is there any good reason to distinguish the two?

            regards, tom lane

Tom Lane írta:
> Zoltan Boszormenyi <zb@cybertec.at> writes:
>
>> So, I should allow DROP DEFAULT, implement
>> SET DEFAULT GENERATED ALWAYS AS
>> and modify the catalog so the GENERATED property
>> is part of pg_attrdef.
>>
>
> Sounds good.
>
>
>> What about IDENTITY?
>> Should it also be part of pg_attrdef? There are two ways
>> to implement it: have or don't have a notion of it.
>> The latter would treat GENERATED BY DEFAULT AS IDENTITY
>> the same as SERIAL.
>>
>
> Is there any good reason to distinguish the two?
>

Yes. Plain SERIALs can be updated with given values
whereas IDENTITY columns cannot. And there is the
difference between GENERATED and GENERATED IDENTITY:
GENERATED columns can updated with DEFAULT
values, IDENTITY columns cannot. I strictly have to
distinguish IDENTITY from both GENERATED and
plain SERIALs.

>             regards, tom lane
>

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

Zoltan Boszormenyi <zb@cybertec.at> writes:
> Tom Lane �rta:
>>> The latter would treat GENERATED BY DEFAULT AS IDENTITY
>>> the same as SERIAL.

>> Is there any good reason to distinguish the two?

> Yes. Plain SERIALs can be updated with given values
> whereas IDENTITY columns cannot.

Really?  How is pg_dump going to deal with that?

            regards, tom lane

Tom Lane írta:
> Zoltan Boszormenyi <zb@cybertec.at> writes:
>
>> Tom Lane írta:
>>
>>>> The latter would treat GENERATED BY DEFAULT AS IDENTITY
>>>> the same as SERIAL.
>>>>
>
>
>>> Is there any good reason to distinguish the two?
>>>
>
>
>> Yes. Plain SERIALs can be updated with given values
>> whereas IDENTITY columns cannot.
>>
>
> Really?  How is pg_dump going to deal with that?
>
>             regards, tom lane
>

It emits ALTER TABLE ... SET GENERATED AS IDENTITY
after ALTER SEQUENCE OWNED BY and if any of the
columns is IDENTITY or GENERATED then it emits
COPY OVERRIDING SYSTEM VALUE in my patch already.

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

Zoltan Boszormenyi <zb@cybertec.at> writes:
> Tom Lane �rta:
>> Zoltan Boszormenyi <zb@cybertec.at> writes:
>>> Yes. Plain SERIALs can be updated with given values
>>> whereas IDENTITY columns cannot.
>>
>> Really?  How is pg_dump going to deal with that?

> It emits ALTER TABLE ... SET GENERATED AS IDENTITY
> after ALTER SEQUENCE OWNED BY and if any of the
> columns is IDENTITY or GENERATED then it emits
> COPY OVERRIDING SYSTEM VALUE in my patch already.

And you fail to see the irony in that?  You might as well just
admit that it's OK to update an identity column.

            regards, tom lane

Tom Lane írta:
> Zoltan Boszormenyi <zb@cybertec.at> writes:
>
>> So, I should allow DROP DEFAULT, implement
>> SET DEFAULT GENERATED ALWAYS AS
>> and modify the catalog so the GENERATED property
>> is part of pg_attrdef.
>>
>
> Sounds good.
>

Finally here it is.

>> What about IDENTITY?
>> Should it also be part of pg_attrdef? There are two ways
>> to implement it: have or don't have a notion of it.
>> The latter would treat GENERATED BY DEFAULT AS IDENTITY
>> the same as SERIAL.
>>
>
> Is there any good reason to distinguish the two?
>

Actually, I needed to have a flag for IDENTITY
but not for the reason above. I need it to distinguish
between GENERATED ALWAYS AS IDENTITY
and GENERATED ALWAYS AS ( expr ).

Changes:
- Rewritten the GENERATED/IDENTITY flags to be part of the default
pg_attrdef
   This made the patch MUCH smaller.
- SERIALs are now the same as INTEGER GENERATED BY DEFAULT AS IDENTITY
- Allow DROP DEFAULT on GENERATED/IDENTITY columns
- Implemented SET GENERATED ALWAYS AS
- Modified syntax of SET GENERATED {ALWAYS | BY DEFAULT} AS IDENTITY
    so it reads as SET IDENTITY GENERATED {ALWAYS | BY DEFAULT}
    so compiling gram.y/gram.c doesn't give me errors.
    This DDL statement isn't part of SQL:2003 so it might be accepted
    as a PostgreSQL extension.
- Modified behaviour of SET IDENTITY to also restore the DEFAULT
    expression. Someone might have done did a DROP DEFAULT before
    but kept the OWNED sequence.
- Fixed behaviour of GENERATED columns regarding
    INSERT ... OVERRIDING SYSTEM VALUE and
    only those GENERATED columns get UPDATEd that
    are either explicitly modified with SET column = DEFAULT
    or one of their referenced columns are modified.
- Testcase and documentation is modified to reflect the above.

Please, review.

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

Zoltan Boszormenyi írta:
> Tom Lane írta:
>> Zoltan Boszormenyi <zb@cybertec.at> writes:
>>
>>> So, I should allow DROP DEFAULT, implement
>>> SET DEFAULT GENERATED ALWAYS AS
>>> and modify the catalog so the GENERATED property
>>> is part of pg_attrdef.
>>>
>>
>> Sounds good.
>>
>
> Finally here it is.
>
>>> What about IDENTITY?
>>> Should it also be part of pg_attrdef? There are two ways
>>> to implement it: have or don't have a notion of it.
>>> The latter would treat GENERATED BY DEFAULT AS IDENTITY
>>> the same as SERIAL.
>>>
>>
>> Is there any good reason to distinguish the two?
>>
>
> Actually, I needed to have a flag for IDENTITY
> but not for the reason above. I need it to distinguish
> between GENERATED ALWAYS AS IDENTITY
> and GENERATED ALWAYS AS ( expr ).
>
> Changes:
> - Rewritten the GENERATED/IDENTITY flags to be part of the default
> pg_attrdef
>   This made the patch MUCH smaller.
> - SERIALs are now the same as INTEGER GENERATED BY DEFAULT AS IDENTITY
> - Allow DROP DEFAULT on GENERATED/IDENTITY columns
> - Implemented SET GENERATED ALWAYS AS
> - Modified syntax of SET GENERATED {ALWAYS | BY DEFAULT} AS IDENTITY
>    so it reads as SET IDENTITY GENERATED {ALWAYS | BY DEFAULT}
>    so compiling gram.y/gram.c doesn't give me errors.
>    This DDL statement isn't part of SQL:2003 so it might be accepted
>    as a PostgreSQL extension.
> - Modified behaviour of SET IDENTITY to also restore the DEFAULT
>    expression. Someone might have done did a DROP DEFAULT before
>    but kept the OWNED sequence.
> - Fixed behaviour of GENERATED columns regarding
>    INSERT ... OVERRIDING SYSTEM VALUE and
>    only those GENERATED columns get UPDATEd that
>    are either explicitly modified with SET column = DEFAULT
>    or one of their referenced columns are modified.
> - Testcase and documentation is modified to reflect the above.

- Also allowed UPDATE on IDENTITY columns.

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


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

Zoltan Boszormenyi írta:
> Andrew Dunstan írta:
>> Florian G. Pflug wrote:
>>>>>>
>>>>>> bison -y -d  gram.y
>>>>>> conflicts: 2 shift/reduce
>>>
>>> I'ts been quite a time since I last used bison, but as far as I
>>> remember, you can tell it to write a rather details log about
>>> it's analysis of the grammar. That log should include more
>>> detailed information about those conflicts - maybe that helps
>>> to figure out their exact cause, and to find a workaround.
>>>
>>
>> You can almost always get rid of shift/reduce conflicts by unwinding
>> some of the productions - resist the temptation to factor the
>> grammar. The effect of this is to eliminate places where the parser
>> has to decide between shifting and reducing. (This is why, for
>> example, almost all the "drop foo if exists" variants require
>> separate productions rather than using opt_if_exists.)
>>
>> cheers
>>
>> andrew
>
> Thanks. This idea solved one of the two shift/reduce conflicts.
> But the other one can only be solved if I put GENERATED
> into the reserved_keyword set. But the standard spec says
> it's unreserved. Now what should I do with it?

What should I do? Send it. :-)
Someone more familiar with bison can hopefully fix it...
Please, review.

Best regards,
Zoltán

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

Zoltan Boszormenyi wrote:
>>>
>>> You can almost always get rid of shift/reduce conflicts by unwinding
>>> some of the productions - resist the temptation to factor the
>>> grammar. The effect of this is to eliminate places where the parser
>>> has to decide between shifting and reducing. (This is why, for
>>> example, almost all the "drop foo if exists" variants require
>>> separate productions rather than using opt_if_exists.)
>>>
>> Thanks. This idea solved one of the two shift/reduce conflicts.
>> But the other one can only be solved if I put GENERATED
>> into the reserved_keyword set. But the standard spec says
>> it's unreserved. Now what should I do with it?
>
> What should I do? Send it. :-)
> Someone more familiar with bison can hopefully fix it...
> Please, review.
>
>

Yeah, I had a brief look. It's a bit ugly - the remaining conflict is in
the b_expr rules. We do have the filtered_base_yylex() gadget  - not
sure if that can disambiguate for us.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> Zoltan Boszormenyi wrote:
>> Thanks. This idea solved one of the two shift/reduce conflicts.
>> But the other one can only be solved if I put GENERATED
>> into the reserved_keyword set. But the standard spec says
>> it's unreserved. Now what should I do with it?

> Yeah, I had a brief look. It's a bit ugly - the remaining conflict is in
> the b_expr rules. We do have the filtered_base_yylex() gadget  - not
> sure if that can disambiguate for us.

The problem comes from cases like

    colname coltype DEFAULT 5! GENERATED ...

Since b_expr allows postfix operators, it takes one more token of
lookahead than we have to tell if the default expression is "5!"
or "5!GENERATED ...".

There are basically two ways to fix this:

1. Collapse GENERATED ALWAYS and GENERATED BY into single tokens
using filtered_base_yylex.

2. Stop allowing postfix operators in b_expr.

I find #1 a bit icky --- not only does every case added to
filtered_base_yylex slow down parsing a little more, but combined
tokens create rough spots in the parser's behavior.  As an example,
both NULLS and FIRST are allegedly unreserved words, so this should
work:

regression=# create table nulls (x int);
CREATE TABLE
regression=# select first.* from nulls first;
ERROR:  syntax error at or near "first"
LINE 1: select first.* from nulls first;
                                  ^
regression=#

#2 actually seems like a viable alternative: postfix operators aren't
really in common use, and doing this would not only fix GENERATED but
let us de-reserve a few keywords that are currently reserved.  In a
non-exhaustive check I found that COLLATE, DEFERRABLE, and INITIALLY
could become unreserved_keyword if we take out this production:

*** 7429,7436 ****
                  { $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
              | qual_Op b_expr                    %prec Op
                  { $$ = (Node *) makeA_Expr(AEXPR_OP, $1, NULL, $2, @1); }
-             | b_expr qual_Op                    %prec POSTFIXOP
-                 { $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, NULL, @2); }
              | b_expr IS DISTINCT FROM b_expr        %prec IS
                  {
                    $$ = (Node *) makeSimpleA_Expr(AEXPR_DISTINCT, "=", $1, $5, @2);
--- 7550,7555 ----

(Hmm, actually I'm wondering why COLLATE is a keyword at all right
now... but the other two trace directly to the what-comes-after-DEFAULT
issue.)

            regards, tom lane

Tom Lane írta:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> Zoltan Boszormenyi wrote:
>>
>>> Thanks. This idea solved one of the two shift/reduce conflicts.
>>> But the other one can only be solved if I put GENERATED
>>> into the reserved_keyword set. But the standard spec says
>>> it's unreserved. Now what should I do with it?
>>>
>
>
>> Yeah, I had a brief look. It's a bit ugly - the remaining conflict is in
>> the b_expr rules. We do have the filtered_base_yylex() gadget  - not
>> sure if that can disambiguate for us.
>>
>
> The problem comes from cases like
>
>     colname coltype DEFAULT 5! GENERATED ...
>
> Since b_expr allows postfix operators, it takes one more token of
> lookahead than we have to tell if the default expression is "5!"
> or "5!GENERATED ...".
>
> There are basically two ways to fix this:
>
> 1. Collapse GENERATED ALWAYS and GENERATED BY into single tokens
> using filtered_base_yylex.
>
> 2. Stop allowing postfix operators in b_expr.
>
> I find #1 a bit icky --- not only does every case added to
> filtered_base_yylex slow down parsing a little more, but combined
> tokens create rough spots in the parser's behavior.  As an example,
> both NULLS and FIRST are allegedly unreserved words, so this should
> work:
>
> regression=# create table nulls (x int);
> CREATE TABLE
> regression=# select first.* from nulls first;
> ERROR:  syntax error at or near "first"
> LINE 1: select first.* from nulls first;
>                                   ^
> regression=#
>
> #2 actually seems like a viable alternative: postfix operators aren't
> really in common use, and doing this would not only fix GENERATED but
> let us de-reserve a few keywords that are currently reserved.  In a
> non-exhaustive check I found that COLLATE, DEFERRABLE, and INITIALLY
> could become unreserved_keyword if we take out this production:
>
> *** 7429,7436 ****
>                   { $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
>               | qual_Op b_expr                    %prec Op
>                   { $$ = (Node *) makeA_Expr(AEXPR_OP, $1, NULL, $2, @1); }
> -             | b_expr qual_Op                    %prec POSTFIXOP
> -                 { $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, NULL, @2); }
>               | b_expr IS DISTINCT FROM b_expr        %prec IS
>                   {
>                     $$ = (Node *) makeSimpleA_Expr(AEXPR_DISTINCT, "=", $1, $5, @2);
> --- 7550,7555 ----
>
> (Hmm, actually I'm wondering why COLLATE is a keyword at all right
> now... but the other two trace directly to the what-comes-after-DEFAULT
> issue.)
>
>             regards, tom lane
>

I looked at it a bit and tried to handle DEFAULT differently from
other column constaints. Basically I did what the standard says:

<column definition> ::=
  <column name> [ <data type or domain name> ]
      [ <default clause> | <identity column specification> | <generation
clause> ]
      [ <column constraint definition>... ]
      [ <collate clause> ]

So DEFAULT and GENERATED {BY DEFAULT | ALWAYS } AS
{ IDENTITY| GENERATED} is made mutually exclusive in the grammar
and these must come before any other column constraints.

This have one possible problem and one fix. The problem is that
the DEFAULT clause cannot be mixed with other constraints any more,
hence breaking some regression tests and lazy deployment.
BTW the PostgreSQL documentation already says that DEFAULT
must come after the type specification.

The other is that specifying DEFAULT as a named constraint isn't allowed
any more. See the confusion below. PostgreSQL happily accepts
but forgets about the name I gave to the DEFAULT clause.

db=# create table aaa (id integer not null constraint my_default default
5, t text);
CREATE TABLE
db=# \d aaa
          Tábla "public.aaa"
 Oszlop |  Típus  |      Módosító
--------+---------+--------------------
 id     | integer | not null default 5
 t      | text    |

db=# alter table aaa drop constraint my_default ;
ERROR:  constraint "my_default" does not exist

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

Zoltan Boszormenyi írta:
> Tom Lane írta:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>
>>> Zoltan Boszormenyi wrote:
>>>
>>>> Thanks. This idea solved one of the two shift/reduce conflicts.
>>>> But the other one can only be solved if I put GENERATED
>>>> into the reserved_keyword set. But the standard spec says
>>>> it's unreserved. Now what should I do with it?
>>>>
>>
>>
>>> Yeah, I had a brief look. It's a bit ugly - the remaining conflict
>>> is in the b_expr rules. We do have the filtered_base_yylex() gadget
>>> - not sure if that can disambiguate for us.
>>>
>>
>> The problem comes from cases like
>>
>>     colname coltype DEFAULT 5! GENERATED ...
>>
>> Since b_expr allows postfix operators, it takes one more token of
>> lookahead than we have to tell if the default expression is "5!"
>> or "5!GENERATED ...".
>>
>> There are basically two ways to fix this:
>>
>> 1. Collapse GENERATED ALWAYS and GENERATED BY into single tokens
>> using filtered_base_yylex.
>>
>> 2. Stop allowing postfix operators in b_expr.
>>
>> I find #1 a bit icky --- not only does every case added to
>> filtered_base_yylex slow down parsing a little more, but combined
>> tokens create rough spots in the parser's behavior.  As an example,
>> both NULLS and FIRST are allegedly unreserved words, so this should
>> work:
>>
>> regression=# create table nulls (x int);
>> CREATE TABLE
>> regression=# select first.* from nulls first;
>> ERROR:  syntax error at or near "first"
>> LINE 1: select first.* from nulls first;
>>                                   ^
>> regression=#
>>
>> #2 actually seems like a viable alternative: postfix operators aren't
>> really in common use, and doing this would not only fix GENERATED but
>> let us de-reserve a few keywords that are currently reserved.  In a
>> non-exhaustive check I found that COLLATE, DEFERRABLE, and INITIALLY
>> could become unreserved_keyword if we take out this production:
>>
>> *** 7429,7436 ****
>>                   { $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3,
>> @2); }
>>               | qual_Op b_expr                    %prec Op
>>                   { $$ = (Node *) makeA_Expr(AEXPR_OP, $1, NULL, $2,
>> @1); }
>> -             | b_expr qual_Op                    %prec POSTFIXOP
>> -                 { $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, NULL,
>> @2); }
>>               | b_expr IS DISTINCT FROM b_expr        %prec IS
>>                   {
>>                     $$ = (Node *) makeSimpleA_Expr(AEXPR_DISTINCT,
>> "=", $1, $5, @2);
>> --- 7550,7555 ----
>>
>> (Hmm, actually I'm wondering why COLLATE is a keyword at all right
>> now... but the other two trace directly to the what-comes-after-DEFAULT
>> issue.)
>>
>>             regards, tom lane
>>
>
> I looked at it a bit and tried to handle DEFAULT differently from
> other column constaints. Basically I did what the standard says:
>
> <column definition> ::=
>  <column name> [ <data type or domain name> ]
>      [ <default clause> | <identity column specification> |
> <generation clause> ]
>      [ <column constraint definition>... ]
>      [ <collate clause> ]
>
> So DEFAULT and GENERATED {BY DEFAULT | ALWAYS } AS
> { IDENTITY| GENERATED} is made mutually exclusive in the grammar
> and these must come before any other column constraints.
>
> This have one possible problem and one fix. The problem is that
> the DEFAULT clause cannot be mixed with other constraints any more,
> hence breaking some regression tests and lazy deployment.
> BTW the PostgreSQL documentation already says that DEFAULT
> must come after the type specification.
>
> The other is that specifying DEFAULT as a named constraint isn't allowed
> any more. See the confusion below. PostgreSQL happily accepts
> but forgets about the name I gave to the DEFAULT clause.
>
> db=# create table aaa (id integer not null constraint my_default
> default 5, t text);
> CREATE TABLE
> db=# \d aaa
>          Tábla "public.aaa"
> Oszlop |  Típus  |      Módosító
> --------+---------+--------------------
> id     | integer | not null default 5
> t      | text    |
>
> db=# alter table aaa drop constraint my_default ;
> ERROR:  constraint "my_default" does not exist
>

Here's what it looks like now. Another side effect is that
the check for conflicting DEFAULT clauses can now be
deleted from analyze.c.

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

Tom Lane wrote:
> The problem comes from cases like
>
>     colname coltype DEFAULT 5! GENERATED ...
>
> Since b_expr allows postfix operators, it takes one more token of
> lookahead than we have to tell if the default expression is "5!"
> or "5!GENERATED ...".
>
> There are basically two ways to fix this:
>
> 1. Collapse GENERATED ALWAYS and GENERATED BY into single tokens
> using filtered_base_yylex.
>
> 2. Stop allowing postfix operators in b_expr.
>
>

I can't think of any good reason why we need postfix operators at all.
Plenty of languages do quite happily without them, and where they make
some sense (e.g. in C) they do so because of their side effect, which
doesn't seem relevant here.

So I vote for #2 unless it will break too much legacy stuff. You should
always be able to replace "operand postop" with a function call anyway -
it's arguably just syntactic sugar.

cheers

andrew

Andrew Dunstan wrote:
> Tom Lane wrote:
> >The problem comes from cases like
> >
> >    colname coltype DEFAULT 5! GENERATED ...
> >
> >Since b_expr allows postfix operators, it takes one more token of
> >lookahead than we have to tell if the default expression is "5!"
> >or "5!GENERATED ...".
> >
> >There are basically two ways to fix this:
> >
> >1. Collapse GENERATED ALWAYS and GENERATED BY into single tokens
> >using filtered_base_yylex.
> >
> >2. Stop allowing postfix operators in b_expr.
>
> I can't think of any good reason why we need postfix operators at all.
> Plenty of languages do quite happily without them, and where they make
> some sense (e.g. in C) they do so because of their side effect, which
> doesn't seem relevant here.
>
> So I vote for #2 unless it will break too much legacy stuff. You should
> always be able to replace "operand postop" with a function call anyway -
> it's arguably just syntactic sugar.

Is it not enough to enclose the expression in parentheses?  ISTM that
this rule covers this:

c_expr:
    | '(' a_expr ')' opt_indirection


BTW I just noticed this bug in the comment above a_expr:

 * Note that '(' a_expr ')' is a b_expr, so an unrestricted expression can
 * always be used by surrounding it with parens.

It is wrong because it's not a b_expr, but a c_expr.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:
> BTW I just noticed this bug in the comment above a_expr:

>  * Note that '(' a_expr ')' is a b_expr, so an unrestricted expression can
>  * always be used by surrounding it with parens.

> It is wrong because it's not a b_expr, but a c_expr.

Well, it's right in context because the comment is discussing the
difference between a_expr and b_expr.  Also, a c_expr is-a b_expr.

If anyone seriously wants to propose removing postfix ops from b_expr,
we'd better take it up on someplace more widely read than -patches.

            regards, tom lane

Zoltan Boszormenyi írta:
> Martijn van Oosterhout írta:
>> On Thu, Apr 19, 2007 at 11:19:40AM +0200, Zoltan Boszormenyi wrote:
>>
>>>> The problem comes from cases like
>>>>
>>>>     colname coltype DEFAULT 5! GENERATED ...
>>>>
>>>> Since b_expr allows postfix operators, it takes one more token of
>>>> lookahead than we have to tell if the default expression is "5!"
>>>> or "5!GENERATED ...".
>>>>
>>
>> ISTM that as long as:
>>
>>  colname coltype DEFAULT (5!) GENERATED ...
>>
>> works I don't see why it would be a problem to require the parentheses
>> in this case. Postfis operators are not going to be that common here I
>> think.
>>
>> Have a nice day,
>>
>
> You mean like this one?
> ------------------------------------------------------------------------
> *** gram.y.old  2007-04-20 09:23:16.000000000 +0200
> --- gram.y      2007-04-20 09:25:34.000000000 +0200
> ***************
> *** 7550,7557 ****
>                                { $$ = (Node *) makeA_Expr(AEXPR_OP,
> $2, $1, $3, @2); }
>                        | qual_Op
> b_expr                                        %prec Op
>                                { $$ = (Node *) makeA_Expr(AEXPR_OP,
> $1, NULL, $2, @1); }
> !                       | b_expr
> qual_Op                                        %prec POSTFIXOP
> !                               { $$ = (Node *) makeA_Expr(AEXPR_OP,
> $2, $1, NULL, @2); }
>                        | b_expr IS DISTINCT FROM b_expr
> %prec IS
>                                {
>                                        $$ = (Node *)
> makeSimpleA_Expr(AEXPR_DISTINCT, "=", $1, $5, @2);
> --- 7550,7557 ----
>                                { $$ = (Node *) makeA_Expr(AEXPR_OP,
> $2, $1, $3, @2); }
>                        | qual_Op
> b_expr                                        %prec Op
>                                { $$ = (Node *) makeA_Expr(AEXPR_OP,
> $1, NULL, $2, @1); }
> !                       | '(' b_expr qual_Op
> ')'                             %prec POSTFIXOP
> !                               { $$ = (Node *) makeA_Expr(AEXPR_OP,
> $3, $2, NULL, @3); }
>                        | b_expr IS DISTINCT FROM b_expr
> %prec IS
>                                {
>                                        $$ = (Node *)
> makeSimpleA_Expr(AEXPR_DISTINCT, "=", $1, $5, @2);
> ------------------------------------------------------------------------
>
> This change alone brings 13 reduce/reduce conflicts.

On the other hand, marking GENERATED as %right
solves this issue. I hope it's an acceptable solution.

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

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

Zoltan Boszormenyi wrote:
> On the other hand, marking GENERATED as %right
> solves this issue. I hope it's an acceptable solution.
>

If anything I should have thought it would be marked %nonassoc.

cheers

andrew

Andrew Dunstan írta:
> Zoltan Boszormenyi wrote:
>> On the other hand, marking GENERATED as %right
>> solves this issue. I hope it's an acceptable solution.
>>
>
> If anything I should have thought it would be marked %nonassoc.
>
> cheers
>
> andrew

That works, too.

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

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

Zoltan Boszormenyi <zb@cybertec.at> writes:
> Andrew Dunstan �rta:
>> Zoltan Boszormenyi wrote:
>>> On the other hand, marking GENERATED as %right
>>> solves this issue. I hope it's an acceptable solution.
>>
>> If anything I should have thought it would be marked %nonassoc.

> That works, too.

[ a bit alarmed... ]  This is only going to be an acceptable solution
if you can explain *exactly why* it works.  The general story with
associativity/precedence declarations is that you are making bison
resolve ambiguous situations in particular ways.  If you don't have a
100% clear understanding of what the ambiguity is and why this is the
right way to resolve it, you are probably creating a bigger problem.

            regards, tom lane

Tom Lane írta:
> Zoltan Boszormenyi <zb@cybertec.at> writes:
>
>> Andrew Dunstan írta:
>>
>>> Zoltan Boszormenyi wrote:
>>>
>>>> On the other hand, marking GENERATED as %right
>>>> solves this issue. I hope it's an acceptable solution.
>>>>
>>> If anything I should have thought it would be marked %nonassoc.
>>>
>
>
>> That works, too.
>>
>
> [ a bit alarmed... ]  This is only going to be an acceptable solution
> if you can explain *exactly why* it works.  The general story with
> associativity/precedence declarations is that you are making bison
> resolve ambiguous situations in particular ways.  If you don't have a
> 100% clear understanding of what the ambiguity is and why this is the
> right way to resolve it, you are probably creating a bigger problem.
>
>             regards, tom lane
>

As far as I remember from my math classes, associativity is
the rules about the way brackets are allowed to be used.
Say, multiplication is two-way associative, i.e.:

a * b * c == (a * b) * c == a * (b * c)

If it was only left associative, the line below would be true:

a * b * c == (a * b) * c != a * (b * c)

Similarly, if it was only right-associative, this would be true:

a * b * c == a * (b * c) != (a * b) * c

Precedence is about the implicit bracketing above
two operators, i.e.

a * b + c * d == (a * b) + (c * d)

(Sorry for the poor explanation, my math classes weren't in English.)

So, before marking, bison was able to do this association:

colname coltype ( DEFAULT 5! GENERATED ) ALWAYS ...

after marking GENERATED as %right, it can only do this:

colname coltype DEFAULT 5! ( GENERATED ALWAYS ... )

With marking GENERATED as %nonassoc, it cannot do either,
leaving the only option for associating DEFAULT as:

colname coltype (DEFAULT 5!)  (GENERATED) ALWAYS ...

So, do any of these cause any problems?

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

Zoltan Boszormenyi <zb@cybertec.at> writes:
> Tom Lane �rta:
>> [ a bit alarmed... ]  This is only going to be an acceptable solution
>> if you can explain *exactly why* it works.  The general story with
>> associativity/precedence declarations is that you are making bison
>> resolve ambiguous situations in particular ways.

> So, before marking, bison was able to do this association:
>
> colname coltype ( DEFAULT 5! GENERATED ) ALWAYS ...
>
> after marking GENERATED as %right, it can only do this:
>
> colname coltype DEFAULT 5! ( GENERATED ALWAYS ... )
>
> With marking GENERATED as %nonassoc, it cannot do either,
> leaving the only option for associating DEFAULT as:
>
> colname coltype (DEFAULT 5!)  (GENERATED) ALWAYS ...

Well, as I was saying, safe use of these options requires knowing
exactly what you're doing, and evidently you don't :-(.  The above
explanation has got about nothing to do with what Bison really does
with associativity/precedence; you need to read the precedence pages
in the Bison manual.

The reason your patch makes it appear to work is not associativity;
it is that you assigned GENERATED a precedence lower than POSTFIXOP.
This means that when Bison is considering whether to reduce a
postfix-operator rule, and GENERATED is the next token, it'll choose
to reduce.  The problem is that that isn't necessarily the right
action; in particular, it makes GENERATED act differently from other
identifiers.  Remember that the whole point here is to keep GENERATED
a non-reserved word.  Supposing that someone had a column named
GENERATED, your patch would make these queries parse differently:

    select x ! generated from ...

    select x ! y from ...

So I think attaching a precedence to the GENERATED keyword is dangerous.

            regards, tom lane

Tom Lane wrote:
>
> So I think attaching a precedence to the GENERATED keyword is dangerous.
>
>

Especially when we have a good workaround which would just require use
of ()  around certain postfix-operator expressions.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> So I think attaching a precedence to the GENERATED keyword is dangerous.

> Especially when we have a good workaround which would just require use
> of ()  around certain postfix-operator expressions.

Yeah, I'm leaning to the idea that removing postfix operators from
b_expr is the least bad solution.

One thing that would have to be looked at is the rules in ruleutils.c
for suppressing "unnecessary" parentheses when reverse-listing
parsetrees.  It might be safest to just never suppress them around a
postfix operator.

            regards, tom lane

Tom Lane írta:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> Tom Lane wrote:
>>
>>> So I think attaching a precedence to the GENERATED keyword is dangerous.
>>>
>
>
>> Especially when we have a good workaround which would just require use
>> of ()  around certain postfix-operator expressions.
>>
>
> Yeah, I'm leaning to the idea that removing postfix operators from
> b_expr is the least bad solution.
>
> One thing that would have to be looked at is the rules in ruleutils.c
> for suppressing "unnecessary" parentheses when reverse-listing
> parsetrees.  It might be safest to just never suppress them around a
> postfix operator.
>
>             regards, tom lane
>

You mean something like this?

-----------------------------------------------------
***************
*** 4138,4157 ****
        Oid                     opno = expr->opno;
        List       *args = expr->args;

-       if (!PRETTY_PAREN(context))
-               appendStringInfoChar(buf, '(');
        if (list_length(args) == 2)
        {
                /* binary operator */
                Node       *arg1 = (Node *) linitial(args);
                Node       *arg2 = (Node *) lsecond(args);

                get_rule_expr_paren(arg1, context, true, (Node *) expr);
                appendStringInfo(buf, " %s ",

generate_operator_name(opno,

exprType(arg1),

exprType(arg2)));
                get_rule_expr_paren(arg2, context, true, (Node *) expr);
        }
        else
        {
--- 4095,4118 ----
        Oid                     opno = expr->opno;
        List       *args = expr->args;

        if (list_length(args) == 2)
        {
                /* binary operator */
                Node       *arg1 = (Node *) linitial(args);
                Node       *arg2 = (Node *) lsecond(args);

+               if (!PRETTY_PAREN(context))
+                       appendStringInfoChar(buf, '(');
+
                get_rule_expr_paren(arg1, context, true, (Node *) expr);
                appendStringInfo(buf, " %s ",

generate_operator_name(opno,

exprType(arg1),

exprType(arg2)));
                get_rule_expr_paren(arg2, context, true, (Node *) expr);
+
+               if (!PRETTY_PAREN(context))
+                       appendStringInfoChar(buf, ')');
        }
        else
        {
***************
*** 4169,4194 ****
                switch (optup->oprkind)
                {
                        case 'l':
                                appendStringInfo(buf, "%s ",

generate_operator_name(opno,

InvalidOid,

exprType(arg)));
                                get_rule_expr_paren(arg, context, true,
(Node *) expr);
                                break;
                        case 'r':
                                get_rule_expr_paren(arg, context, true,
(Node *) expr);
                                appendStringInfo(buf, " %s",

generate_operator_name(opno,

exprType(arg),

InvalidOid));
                                break;
                        default:
                                elog(ERROR, "bogus oprkind: %d",
optup->oprkind);
                }
                ReleaseSysCache(tp);
        }
-       if (!PRETTY_PAREN(context))
-               appendStringInfoChar(buf, ')');
  }

  /*
--- 4130,4159 ----
                switch (optup->oprkind)
                {
                        case 'l':
+                               if (!PRETTY_PAREN(context))
+                                       appendStringInfoChar(buf, '(');
                                appendStringInfo(buf, "%s ",

generate_operator_name(opno,

InvalidOid,

exprType(arg)));
                                get_rule_expr_paren(arg, context, true,
(Node *) expr);
+                               if (!PRETTY_PAREN(context))
+                                       appendStringInfoChar(buf, ')');
                                break;
                        case 'r':
+                               appendStringInfoChar(buf, '(');
                                get_rule_expr_paren(arg, context, true,
(Node *) expr);
                                appendStringInfo(buf, " %s",

generate_operator_name(opno,

exprType(arg),

InvalidOid));
+                               appendStringInfoChar(buf, ')');
                                break;
                        default:
                                elog(ERROR, "bogus oprkind: %d",
optup->oprkind);
                }
                ReleaseSysCache(tp);
        }
  }

  /*
-----------------------------------------------------

It seems to work:

-----------------------------------------------------
$ psql
Welcome to psql 8.3devel, the PostgreSQL interactive terminal.

Type:  \copyright for distribution terms
       \h for help with SQL commands
       \? for help with psql commands
       \g or terminate with semicolon to execute query
       \q to quit

zozo=# create table t1 (i integer default 5! generated always as
identity primary key, t text not null unique);
ERROR:  syntax error at or near "always"
LINE 1: create table t1 (i integer default 5! generated always as id...
                                                        ^
zozo=# create table t1 (i integer default (5!) generated always as
identity primary key, t text not null unique);
NOTICE:  CREATE TABLE will create implicit sequence "t1_i_seq" for
serial column "t1.i"
ERROR:  multiple default values specified for column "i" of table "t1"
zozo=# create table t1 (i integer default (5!) primary key, t text not
null unique);
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "t1_pkey"
for table "t1"
NOTICE:  CREATE TABLE / UNIQUE will create implicit index "t1_t_key" for
table "t1"
CREATE TABLE
zozo=# \d t1
                  Table "public.t1"
 Column |  Type   |            Modifiers
--------+---------+----------------------------------
 i      | integer | not null default ((5)::bigint !)
 t      | text    | not null
Indexes:
    "t1_pkey" PRIMARY KEY, btree (i)
    "t1_t_key" UNIQUE, btree (t)

zozo=# \q
$ pg_dump
...
--
-- Name: t1; Type: TABLE; Schema: public; Owner: zozo; Tablespace:
--

CREATE TABLE t1 (
    i integer DEFAULT ((5)::bigint !) NOT NULL,
    t text NOT NULL
);

...

--
-- PostgreSQL database dump complete
--
-----------------------------------------------------

So, if I send the patch again (POSTFIXOP deleted from b_expr)
with the above added, will it get accepted?

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

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

Hi,

here's the patch with the modifications suggested by Tom Lane.
The postfix rule was deleted from b_expr and the reverse parsing
in ruleutils.c::get_oper_expr() always puts parentheses around
postfix operators.

Other changes:
- OVERRIDING SYSTEM VALUE in COPY can appear
   at any place in the option list.
- pg_dump was modified accordingly
- \copy built-in in psql now also accepts OVERRIDING SYSTEM VALUE
- documentation and testcase updates

Please, review.

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

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

And here it is attached. Sorry.

Zoltan Boszormenyi írta:
> Hi,
>
> here's the patch with the modifications suggested by Tom Lane.
> The postfix rule was deleted from b_expr and the reverse parsing
> in ruleutils.c::get_oper_expr() always puts parentheses around
> postfix operators.
>
> Other changes:
> - OVERRIDING SYSTEM VALUE in COPY can appear
>   at any place in the option list.
> - pg_dump was modified accordingly
> - \copy built-in in psql now also accepts OVERRIDING SYSTEM VALUE
> - documentation and testcase updates
>
> Please, review.
>
> Best regards,
> Zoltán Böszörményi
>


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

Hi,

some last changes. Really. :-)

I made ALTER TABLE symmetric with CREATE TABLE
so the grammar now has:

ALTER TABLE tabname ALTER colname SET GENERATED
   { ALWAYS | BY DEFAULT} AS IDENTITY [ ( sequence options )]

This works intuitively the same as in CREATE TABLE, i.e.
- it creates an OWNED sequence (if the column doesn't already have one)
- it creates or alters the sequence with the given options
- adds the DEFAULT expression with the proper generation behaviour
in one go. I extended the documentation and modified the test case
accordingly.
I also tested that an IDENTITY column can't be created with a type that
cannot be cast from bigint i.e. box. I added it to the test case.

Please, review.

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

Zoltan Boszormenyi írta:
> And here it is attached. Sorry.
>
> Zoltan Boszormenyi írta:
>> Hi,
>>
>> here's the patch with the modifications suggested by Tom Lane.
>> The postfix rule was deleted from b_expr and the reverse parsing
>> in ruleutils.c::get_oper_expr() always puts parentheses around
>> postfix operators.
>>
>> Other changes:
>> - OVERRIDING SYSTEM VALUE in COPY can appear
>>   at any place in the option list.
>> - pg_dump was modified accordingly
>> - \copy built-in in psql now also accepts OVERRIDING SYSTEM VALUE
>> - documentation and testcase updates
>>
>> Please, review.
>>
>> Best regards,
>> Zoltán Böszörményi
>>
>
>
> ------------------------------------------------------------------------
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster
>


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

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,
>
> some last changes. Really. :-)
>
> I made ALTER TABLE symmetric with CREATE TABLE
> so the grammar now has:
>
> ALTER TABLE tabname ALTER colname SET GENERATED
>    { ALWAYS | BY DEFAULT} AS IDENTITY [ ( sequence options )]
>
> This works intuitively the same as in CREATE TABLE, i.e.
> - it creates an OWNED sequence (if the column doesn't already have one)
> - it creates or alters the sequence with the given options
> - adds the DEFAULT expression with the proper generation behaviour
> in one go. I extended the documentation and modified the test case
> accordingly.
> I also tested that an IDENTITY column can't be created with a type that
> cannot be cast from bigint i.e. box. I added it to the test case.
>
> Please, review.
>
> Best regards,
> Zolt?n B?sz?rm?nyi
>
> Zoltan Boszormenyi ?rta:
> > And here it is attached. Sorry.
> >
> > Zoltan Boszormenyi ?rta:
> >> Hi,
> >>
> >> here's the patch with the modifications suggested by Tom Lane.
> >> The postfix rule was deleted from b_expr and the reverse parsing
> >> in ruleutils.c::get_oper_expr() always puts parentheses around
> >> postfix operators.
> >>
> >> Other changes:
> >> - OVERRIDING SYSTEM VALUE in COPY can appear
> >>   at any place in the option list.
> >> - pg_dump was modified accordingly
> >> - \copy built-in in psql now also accepts OVERRIDING SYSTEM VALUE
> >> - documentation and testcase updates
> >>
> >> Please, review.
> >>
> >> Best regards,
> >> Zolt?n B?sz?rm?nyi
> >>
> >
> >
> > ------------------------------------------------------------------------
> >
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 2: Don't 'kill -9' the postmaster
> >
>
>
> --
> ----------------------------------
> Zolt?n B?sz?rm?nyi
> Cybertec Geschwinde & Sch?nig GmbH
> http://www.postgresql.at/
>

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

--
  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. +

Thanks.

But actually it didn't showed up at that page.
For that matter, neither patch showed up on either pgpatches
or pgpatches_hold that you indicated yesterday.

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,
>>
>> some last changes. Really. :-)
>>
>> I made ALTER TABLE symmetric with CREATE TABLE
>> so the grammar now has:
>>
>> ALTER TABLE tabname ALTER colname SET GENERATED
>>    { ALWAYS | BY DEFAULT} AS IDENTITY [ ( sequence options )]
>>
>> This works intuitively the same as in CREATE TABLE, i.e.
>> - it creates an OWNED sequence (if the column doesn't already have one)
>> - it creates or alters the sequence with the given options
>> - adds the DEFAULT expression with the proper generation behaviour
>> in one go. I extended the documentation and modified the test case
>> accordingly.
>> I also tested that an IDENTITY column can't be created with a type that
>> cannot be cast from bigint i.e. box. I added it to the test case.
>>
>> Please, review.
>>
>> Best regards,
>> Zolt?n B?sz?rm?nyi
>>
>> Zoltan Boszormenyi ?rta:
>>
>>> And here it is attached. Sorry.
>>>
>>> Zoltan Boszormenyi ?rta:
>>>
>>>> Hi,
>>>>
>>>> here's the patch with the modifications suggested by Tom Lane.
>>>> The postfix rule was deleted from b_expr and the reverse parsing
>>>> in ruleutils.c::get_oper_expr() always puts parentheses around
>>>> postfix operators.
>>>>
>>>> Other changes:
>>>> - OVERRIDING SYSTEM VALUE in COPY can appear
>>>>   at any place in the option list.
>>>> - pg_dump was modified accordingly
>>>> - \copy built-in in psql now also accepts OVERRIDING SYSTEM VALUE
>>>> - documentation and testcase updates
>>>>
>>>> Please, review.
>>>>
>>>> Best regards,
>>>> Zolt?n B?sz?rm?nyi
>>>>
>>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>> ---------------------------(end of broadcast)---------------------------
>>> TIP 2: Don't 'kill -9' the postmaster
>>>
>>>
>> --
>> ----------------------------------
>> Zolt?n B?sz?rm?nyi
>> Cybertec Geschwinde & Sch?nig GmbH
>> http://www.postgresql.at/
>>
>>
>
> [ application/x-tar is not supported, skipping... ]
>
>


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

Zoltan Boszormenyi wrote:
> Thanks.
>
> But actually it didn't showed up at that page.
> For that matter, neither patch showed up on either pgpatches
> or pgpatches_hold that you indicated yesterday.

My apologies.  Something was misconfigured on my end.  The web pages are
fixed now.

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


>
> 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,
> >>
> >> some last changes. Really. :-)
> >>
> >> I made ALTER TABLE symmetric with CREATE TABLE
> >> so the grammar now has:
> >>
> >> ALTER TABLE tabname ALTER colname SET GENERATED
> >>    { ALWAYS | BY DEFAULT} AS IDENTITY [ ( sequence options )]
> >>
> >> This works intuitively the same as in CREATE TABLE, i.e.
> >> - it creates an OWNED sequence (if the column doesn't already have one)
> >> - it creates or alters the sequence with the given options
> >> - adds the DEFAULT expression with the proper generation behaviour
> >> in one go. I extended the documentation and modified the test case
> >> accordingly.
> >> I also tested that an IDENTITY column can't be created with a type that
> >> cannot be cast from bigint i.e. box. I added it to the test case.
> >>
> >> Please, review.
> >>
> >> Best regards,
> >> Zolt?n B?sz?rm?nyi
> >>
> >> Zoltan Boszormenyi ?rta:
> >>
> >>> And here it is attached. Sorry.
> >>>
> >>> Zoltan Boszormenyi ?rta:
> >>>
> >>>> Hi,
> >>>>
> >>>> here's the patch with the modifications suggested by Tom Lane.
> >>>> The postfix rule was deleted from b_expr and the reverse parsing
> >>>> in ruleutils.c::get_oper_expr() always puts parentheses around
> >>>> postfix operators.
> >>>>
> >>>> Other changes:
> >>>> - OVERRIDING SYSTEM VALUE in COPY can appear
> >>>>   at any place in the option list.
> >>>> - pg_dump was modified accordingly
> >>>> - \copy built-in in psql now also accepts OVERRIDING SYSTEM VALUE
> >>>> - documentation and testcase updates
> >>>>
> >>>> Please, review.
> >>>>
> >>>> Best regards,
> >>>> Zolt?n B?sz?rm?nyi
> >>>>
> >>>>
> >>> ------------------------------------------------------------------------
> >>>
> >>>
> >>> ---------------------------(end of broadcast)---------------------------
> >>> TIP 2: Don't 'kill -9' the postmaster
> >>>
> >>>
> >> --
> >> ----------------------------------
> >> Zolt?n B?sz?rm?nyi
> >> Cybertec Geschwinde & Sch?nig GmbH
> >> http://www.postgresql.at/
> >>
> >>
> >
> > [ application/x-tar is not supported, skipping... ]
> >
> >
>
>
> --
> ----------------------------------
> Zolt?n B?sz?rm?nyi
> Cybertec Geschwinde & Sch?nig GmbH
> http://www.postgresql.at/
>
>
> ---------------------------(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

--
  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. +