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/


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

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

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

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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

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/


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

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/


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq