Re: [HACKERS] Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
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
Re: [HACKERS] Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
Alvaro Herrera <[EMAIL PROTECTED]> 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
Re: [HACKERS] Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
Zoltan Boszormenyi írta: Tom Lane írta: Andrew Dunstan <[EMAIL PROTECTED]> 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: ::= [ ] [ | | ] [ ... ] [ ] 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/ psql-serial-41.diff.gz Description: Unix tar archive ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
Tom Lane írta: Andrew Dunstan <[EMAIL PROTECTED]> 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: ::= [ ] [ | | 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
Re: [HACKERS] Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
Andrew Dunstan <[EMAIL PROTECTED]> 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
Re: [HACKERS] Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
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
Re: [HACKERS] Re: IDENTITY/GENERATED v36 Re: [PATCHES] Final version of IDENTITY/GENERATED patch
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/ psql-serial-40.diff.gz Description: Unix tar archive ---(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