Skip site navigation (1) Skip section navigation (2)

Re: [HACKERS] parser dilemma

From: Zoltan Boszormenyi <zb(at)cybertec(dot)at>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, Martijn van Oosterhout <kleptog(at)svana(dot)org>, List pgsql-patches <pgsql-patches(at)postgresql(dot)org>, Alvaro Herrera <alvherre(at)commandprompt(dot)com>, "Florian G(dot) Pflug" <fgp(at)phlo(dot)org>, Bruce Momjian <bruce(at)momjian(dot)us>
Subject: Re: [HACKERS] parser dilemma
Date: 2007-04-25 08:31:56
Message-ID: 462F11FC.3030502@cybertec.at (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
Tom Lane írta:
> Andrew Dunstan <andrew(at)dunslane(dot)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/


In response to

Responses

pgsql-hackers by date

Next:From: ITAGAKI TakahiroDate: 2007-04-25 09:16:39
Subject: Load distributed checkpoint V4.1
Previous:From: ITAGAKI TakahiroDate: 2007-04-25 08:27:15
Subject: autovacuum does not start in HEAD

pgsql-patches by date

Next:From: ITAGAKI TakahiroDate: 2007-04-25 09:16:39
Subject: Load distributed checkpoint V4.1
Previous:From: ITAGAKI TakahiroDate: 2007-04-25 08:27:15
Subject: autovacuum does not start in HEAD

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group