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/


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

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


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

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

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

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/


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

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

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

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

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

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

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

http://archives.postgresql.org

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


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

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/


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

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