Обсуждение: Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
От
Zoltan Boszormenyi
Дата:
Hi,
Zoltan Boszormenyi írta:
> 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.
>
I just realized that by treating SERIAL the same as
IDENTITY GENERATED BY DEFAULT, I incidentally
broke the possibility of multiple SERIALs in the same table.
I rewrote the patch so instead of two BOOL flags,
I now have only one CHAR flag:
- ' ' says it's a simple DEFAULT expression
- 'i' says it's GENERATED ALWAYS AS IDENTITY
- 'g' says it's GENERATED ALWAYS AS ( expr )
Apart from making the patch a bit smaller again, checking only
for 'i' still allows multiple SERIALs in the same table but lets
disallowing multiple GENERATED ALWAYS AS IDENTITY.
Thinking a bit about it, is it desired to disallow multiple GENERATED
ALWAYS AS IDENTITY fields? It's just a twisted SERIAL anyway.
And it was said many times that it's not an advantage to blindly
follow the standard.
Also, DROP IDENTITY is equivalent with SET DEFAULT
nextval('owned_sequence') iff the field has an OWNED
sequence and it was GENERATED ALWAYS AS IDENTITY before.
Considering that SERIAL is a macro, and SET/DROP DEFAULT is allowed
on IDENTITY/GENERATED columns per Tom's request,
should I keep this statement?
Also, the current grammar is made to give a syntax error
if you say "colname type GENERATED BY DEFAULT AS ( expr )".
But it makes the grammar unbalanced, and gives me:
bison -y -d gram.y
conflicts: 2 shift/reduce
Is there a good solution to this?
I post the new patch after someone answers those questions for me.
--
----------------------------------
Zoltán Böszörményi
Cybertec Geschwinde & Schönig GmbH
http://www.postgresql.at/
Zoltan Boszormenyi <zb@cybertec.at> writes:
> Apart from making the patch a bit smaller again, checking only
> for 'i' still allows multiple SERIALs in the same table but lets
> disallowing multiple GENERATED ALWAYS AS IDENTITY.
> Thinking a bit about it, is it desired to disallow multiple GENERATED
> ALWAYS AS IDENTITY fields? It's just a twisted SERIAL anyway.
I don't see the value of disallowing it.
> Also, DROP IDENTITY is equivalent with SET DEFAULT
> nextval('owned_sequence') iff the field has an OWNED
> sequence and it was GENERATED ALWAYS AS IDENTITY before.
> Considering that SERIAL is a macro, and SET/DROP DEFAULT is allowed
> on IDENTITY/GENERATED columns per Tom's request,
> should I keep this statement?
If it's not in the spec I don't see any strong reason to have it...
> Also, the current grammar is made to give a syntax error
> if you say "colname type GENERATED BY DEFAULT AS ( expr )".
> But it makes the grammar unbalanced, and gives me:
> bison -y -d gram.y
> conflicts: 2 shift/reduce
You'll have to fix that. Usually you can get around it by making the
grammar a bit more verbose --- if you were trying to avoid duplication
by means of optional productions, don't do that.
regards, tom lane
Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
От
Zoltan Boszormenyi
Дата:
Tom Lane írta:
> Zoltan Boszormenyi <zb@cybertec.at> writes:
>
>> Apart from making the patch a bit smaller again, checking only
>> for 'i' still allows multiple SERIALs in the same table but lets
>> disallowing multiple GENERATED ALWAYS AS IDENTITY.
>> Thinking a bit about it, is it desired to disallow multiple GENERATED
>> ALWAYS AS IDENTITY fields? It's just a twisted SERIAL anyway.
>>
>
> I don't see the value of disallowing it.
>
I thought so.
>> Also, DROP IDENTITY is equivalent with SET DEFAULT
>> nextval('owned_sequence') iff the field has an OWNED
>> sequence and it was GENERATED ALWAYS AS IDENTITY before.
>> Considering that SERIAL is a macro, and SET/DROP DEFAULT is allowed
>> on IDENTITY/GENERATED columns per Tom's request,
>> should I keep this statement?
>>
>
> If it's not in the spec I don't see any strong reason to have it...
>
It's not. Removed.
>> Also, the current grammar is made to give a syntax error
>> if you say "colname type GENERATED BY DEFAULT AS ( expr )".
>> But it makes the grammar unbalanced, and gives me:
>> bison -y -d gram.y
>> conflicts: 2 shift/reduce
>>
>
> You'll have to fix that. Usually you can get around it by making the
> grammar a bit more verbose --- if you were trying to avoid duplication
> by means of optional productions, don't do that.
>
What do you mean by "making it more verbose"?
"GENERATED BY DEFAULT AS ( expr )" is another
way of saying "DEFAULT expr" but that being similar
to GENERATED ALWAYS AS ( expr ) would make
the users think that it would permit smarter expressions
than simple DEFAULT would allow. My thought was
to disallow this in the grammar.
BTW, thanks for the quick answer.
--
----------------------------------
Zoltán Böszörményi
Cybertec Geschwinde & Schönig GmbH
http://www.postgresql.at/
Zoltan Boszormenyi <zb@cybertec.at> writes:
> "GENERATED BY DEFAULT AS ( expr )" is another
> way of saying "DEFAULT expr" but that being similar
> to GENERATED ALWAYS AS ( expr ) would make
> the users think that it would permit smarter expressions
> than simple DEFAULT would allow. My thought was
> to disallow this in the grammar.
I think you probably want a more specific error message than "syntax
error" for that, so the right thing to do is complain in code rather
than try to make the grammar per se not have a production that accepts it.
regards, tom lane
Re: Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
От
"Florian G. Pflug"
Дата:
Zoltan Boszormenyi wrote: > Tom Lane írta: >> Zoltan Boszormenyi <zb@cybertec.at> writes: >>> Also, the current grammar is made to give a syntax error >>> if you say "colname type GENERATED BY DEFAULT AS ( expr )". >>> But it makes the grammar unbalanced, and gives me: >>> 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. greetings, Florian Pflug
Re: Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
От
Andrew Dunstan
Дата:
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
Re: Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
От
Zoltan Boszormenyi
Дата:
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? Best regards, Zoltán > > ---------------------------(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 > -- ---------------------------------- Zoltán Böszörményi Cybertec Geschwinde & Schönig GmbH http://www.postgresql.at/
Tom Lane írta:
> ...
> 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
>
OK, I take the bullet and send it to -hackers.
For everyone who don't read -patches, let me reiterate the problem
During developing my GENERATED/IDENTITY patches,
a parser problem turned up.
Currently, DEFAULT is handled as a CONSTRAINT by the parser
to be able to write DEFAULT clause and CONSTRAINT clauses
in any order. Handling GENERATED { ALWAYS | BY DEFAULT}
AS { IDENTITY | ( expression ) } syntax in the same way causes
a conflict between DEFAULT and b_expr as discovered by Tom Lane.
He proposed two solutions, quote:
> 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.)
I proposed a third solution, that is actually standard-conforming
and still leaves the possibility of having postfix operators.
The solution was to admit that DEFAULT is not a CONSTRAINT,
hence not mixable with them. The standard has this syntax:
<column definition> ::=<column name> [ <data type or domain name> ] [ <default clause> | <identity column
specification>| <generation
clause> ] [ <column constraint definition>... ] [ <collate clause> ]
This says that DEFAULT | GENERATED ... AS IDENTITY |
GENERATED ALWAYS AS ( expr ) must come after the data type
and before any CONSTRAINTs and the three forms are mutually exclusive.
This can be nicely handled by the parser and the analyzer phase
can save some cycles by not checking for conflicting DEFAULT clauses.
What do people think? Which would be the preferred solution?
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Geschwinde & Schönig GmbH
http://www.postgresql.at/
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, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to litigate.
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.
Best regards
--
----------------------------------
Zoltán Böszörményi
Cybertec Geschwinde & Schönig GmbH
http://www.postgresql.at/
Zoltan Boszormenyi wrote:
> 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.
>
>
No - that's not what you do. All you need to do is remove those 2 lines
from the b_expr rules. The postfix rule will still be in a_expr and the
parenthesized bit is taken care of in the ( a_expr ) rule for c_expr.
cheers
andrew
Andrew Dunstan írta:
> Zoltan Boszormenyi wrote:
>> 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.
>>
>>
>
> No - that's not what you do. All you need to do is remove those 2
> lines from the b_expr rules. The postfix rule will still be in a_expr
> and the parenthesized bit is taken care of in the ( a_expr ) rule for
> c_expr.
>
> cheers
>
> andrew
I just sent a new patch that marks GENERATED as %right,
which also solved the problem.
Best regards,
Zoltán
--
----------------------------------
Zoltán Böszörményi
Cybertec Geschwinde & Schönig GmbH
http://www.postgresql.at/