Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3

Lists: pgsql-hackers
From: Steve Prentice <prentice(at)cisco(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-21 17:31:55
Message-ID: EB83C343-28E1-4DFB-86E5-09BD98E7E2C0@cisco.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello,

This patch is intended to supplement Pavel's patch for named and mixed
notation support in 8.5. This patch makes it so a plpgsql function can
call another function with the same parameter names using the named
parameters notation. Without this patch, the following example will
have a syntax errors:

CREATE FUNCTION fun1(a INT DEFAULT 1) RETURNS INT AS 'SELECT $1'
LANGUAGE SQL;
CREATE FUNCTION fun2(a INT) RETURNS INT AS $$
DECLARE
t INT;
BEGIN
t := fun1(1 as a); -- syntax error: "SELECT fun1(1 as $1 )"
t := fun1(a as a); -- syntax error: "SELECT fun1( $1 as $1 )"
RETURN 0;
END;
$$ LANGUAGE plpgsql;

The patch adds the "AS" keyword to the plpgsql grammar and doesn't
assign an expression parameter to the sql construct if the scalar
follows the AS keyword.

(v1 of this patch was in the "plpgsql + named parameters" thread, but
it didn't include the doc changes.)

-Steve

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 80dbf45..9b99314 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3510,7 +3510,7 @@ PREPARE <replaceable>statement_name</>(text,
timestamp) AS
</para>

<para>
- <emphasis>The substitution mechanism will replace any token that
matches a
+ <emphasis>The substitution mechanism will replace most tokens
that match a
known variable's name.</> This poses various traps for the unwary.
For example, it is a bad idea
to use a variable name that is the same as any table or column name
@@ -3601,9 +3601,29 @@ CONTEXT: SQL statement in PL/PgSQL function
"logfunc2" near line 5
interpreted the <quote>wrong</> way. But it is useful for
clarifying
the intent of potentially-ambiguous code.
</para>
-
+
+ <para>
+ There are two places where variable substitution does not happen.
+ </para>
+
+ <para>
+ Any label following the "AS" keyword is not replace. This allows
passing
+ parameters by name to functions that have parameters of the same
name as
+ the calling function. For example,
+<programlisting>
+ CREATE FUNCTION logfunc(v_logtxt text) RETURNS void AS $$
+ BEGIN
+ INSERT INTO logtable (logtxt) VALUES (v_logtxt);
+ PERFORM tracefunc(v_logtxt AS v_logtxt);
+ END;
+ $$ LANGUAGE plpgsql;
+</programlisting>
+ All occurances of v_logtxt in the function are replaced except the
one
+ following "AS".
+ </para>
+
<para>
- Variable substitution does not happen in the command string given
+ Variable substitution also does not happen in the command string
given
to <command>EXECUTE</> or one of its variants. If you need to
insert a varying value into such a command, do so as part of
constructing the string value, as illustrated in
diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
index 06704cf..647daab 100644
--- a/src/pl/plpgsql/src/gram.y
+++ b/src/pl/plpgsql/src/gram.y
@@ -177,6 +177,7 @@ static List *read_raise_options(void);
* Keyword tokens
*/
%token K_ALIAS
+%token K_AS
%token K_ASSIGN
%token K_BEGIN
%token K_BY
@@ -1977,6 +1978,7 @@ read_sql_construct(int until,
int *endtoken)
{
int tok;
+ int prevtok = 0;
int lno;
PLpgSQL_dstring ds;
int parenlevel = 0;
@@ -1989,7 +1991,7 @@ read_sql_construct(int until,
plpgsql_dstring_init(&ds);
plpgsql_dstring_append(&ds, sqlstart);

- for (;;)
+ for (;;prevtok = tok)
{
tok = yylex();
if (tok == until && parenlevel == 0)
@@ -2034,10 +2036,16 @@ read_sql_construct(int until,
switch (tok)
{
case T_SCALAR:
- snprintf(buf, sizeof(buf), " $%d ",
- assign_expr_param(yylval.scalar->dno,
- params, &nparams));
- plpgsql_dstring_append(&ds, buf);
+ /* A scalar following AS is treated as a label */
+ if (prevtok == K_AS)
+ plpgsql_dstring_append(&ds, yytext);
+ else
+ {
+ snprintf(buf, sizeof(buf), " $%d ",
+ assign_expr_param(yylval.scalar->dno,
+ params, &nparams));
+ plpgsql_dstring_append(&ds, buf);
+ }
break;

case T_ROW:
diff --git a/src/pl/plpgsql/src/scan.l b/src/pl/plpgsql/src/scan.l
index 1917eef..e3a5c45 100644
--- a/src/pl/plpgsql/src/scan.l
+++ b/src/pl/plpgsql/src/scan.l
@@ -149,6 +149,7 @@ param \${digit}+
= { return K_ASSIGN; }
\.\. { return K_DOTDOT; }
alias { return K_ALIAS; }
+as { return K_AS; }
begin { return K_BEGIN; }
by { return K_BY; }
case { return K_CASE; }


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Steve Prentice <prentice(at)cisco(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-21 17:52:59
Message-ID: 12752.1242928379@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Steve Prentice <prentice(at)cisco(dot)com> writes:
> This patch is intended to supplement Pavel's patch for named and mixed
> notation support in 8.5. This patch makes it so a plpgsql function can
> call another function with the same parameter names using the named
> parameters notation.

Well, plpgsql's parsing has always been a kluge, but I think this is
really taking the kluge level a step too far. It's only because AS
is used in so few contexts that this can even pretend to work --- but
there are still an awful lot of contexts where AS is used, and will
likely be more in the future. So I think it's pretty un-future-proof;
and it certainly won't scale to any other contexts where we might wish
that plpsql variables don't get substituted.

It's probably time to bite the bullet and redo the parser as has been
suggested in the past, ie fix things so that the main parser is used.
Ideally I'd like to switch the name resolution priority to be more
Oracle-like, but even if we don't do that it would be a great
improvement to have actual syntactic knowledge behind the lookups.

Just for the record, you'd have to put the same kluge into the T_RECORD
and T_ROW cases if we wanted to do it like this.

regards, tom lane


From: Steve Prentice <prentice(at)cisco(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-05-21 18:46:24
Message-ID: 1B96769F-91E7-4AB9-84C8-0F3E1A796E9B@cisco.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On May 21, 2009, at 10:52 AM, Tom Lane wrote:
> It's probably time to bite the bullet and redo the parser as has been
> suggested in the past, ie fix things so that the main parser is used.
> Ideally I'd like to switch the name resolution priority to be more
> Oracle-like, but even if we don't do that it would be a great
> improvement to have actual syntactic knowledge behind the lookups.

That kind of refactoring is beyond my experience-level with the code,
but I can't say I disagree with your analysis.

> Just for the record, you'd have to put the same kluge into the
> T_RECORD
> and T_ROW cases if we wanted to do it like this.

Patch updated.

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 80dbf45..f8e8ce4 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3510,7 +3510,7 @@ PREPARE <replaceable>statement_name</>(text,
timestamp) AS
</para>

<para>
- <emphasis>The substitution mechanism will replace any token that
matches a
+ <emphasis>The substitution mechanism will replace most tokens
that match a
known variable's name.</> This poses various traps for the
unwary.
For example, it is a bad idea
to use a variable name that is the same as any table or column
name
@@ -3603,7 +3603,27 @@ CONTEXT: SQL statement in PL/PgSQL function
"logfunc2" near line 5
</para>

<para>
- Variable substitution does not happen in the command string given
+ There are two places where variable substitution does not happen.
+ </para>
+
+ <para>
+ Any label following the "AS" keyword is not replaced. This allows
passing
+ parameters by name to functions that have parameters of the same
name as
+ the calling function. For example,
+<programlisting>
+ CREATE FUNCTION logfunc(v_logtxt text) RETURNS void AS $$
+ BEGIN
+ INSERT INTO logtable (logtxt) VALUES (v_logtxt);
+ PERFORM tracefunc(v_logtxt AS v_logtxt);
+ END;
+ $$ LANGUAGE plpgsql;
+</programlisting>
+ All occurances of v_logtxt in the function are replaced except the
one
+ following "AS".
+ </para>
+
+ <para>
+ Variable substitution also does not happen in the command string
given
to <command>EXECUTE</> or one of its variants. If you need to
insert a varying value into such a command, do so as part of
constructing the string value, as illustrated in
diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
index 06704cf..3b4e9b8 100644
--- a/src/pl/plpgsql/src/gram.y
+++ b/src/pl/plpgsql/src/gram.y
@@ -177,6 +177,7 @@ static List
*read_raise_options(void);
* Keyword tokens
*/
%token K_ALIAS
+%token K_AS
%token K_ASSIGN
%token K_BEGIN
%token K_BY
@@ -1977,6 +1978,7 @@ read_sql_construct(int until,
int *endtoken)
{
int tok;
+ int prevtok = 0;
int lno;
PLpgSQL_dstring ds;
int parenlevel = 0;
@@ -1989,7 +1991,7 @@ read_sql_construct(int until,
plpgsql_dstring_init(&ds);
plpgsql_dstring_append(&ds, sqlstart);

- for (;;)
+ for (;;prevtok = tok)
{
tok = yylex();
if (tok == until && parenlevel == 0)
@@ -2031,6 +2033,16 @@ read_sql_construct(int until,
if (plpgsql_SpaceScanned)
plpgsql_dstring_append(&ds, " ");

+ /* A variable following AS is treated as a label */
+ if (prevtok == K_AS &&
+ (tok == T_SCALAR || tok == T_ROW ||
tok == T_RECORD))
+ {
+ plpgsql_dstring_append(&ds, yytext);
+ continue;
+ }
+
switch (tok)
{
case T_SCALAR:
diff --git a/src/pl/plpgsql/src/scan.l b/src/pl/plpgsql/src/scan.l
index 1917eef..e3a5c45 100644
--- a/src/pl/plpgsql/src/scan.l
+++ b/src/pl/plpgsql/src/scan.l
@@ -149,6 +149,7 @@ param \${digit}+
= { return
K_ASSIGN; }
\.\. { return K_DOTDOT; }
alias { return K_ALIAS; }
+as { return
K_AS; }
begin { return K_BEGIN; }
by { return
K_BY; }
case { return K_CASE; }


From: Josh Berkus <josh(at)agliodbs(dot)com>
To: Steve Prentice <prentice(at)cisco(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-21 18:51:14
Message-ID: 4A15A2A2.4070509@agliodbs.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


> The patch adds the "AS" keyword to the plpgsql grammar and doesn't
> assign an expression parameter to the sql construct if the scalar
> follows the AS keyword.

Would it be possible to also support "=" as well as "as"? I believe
that SQL Server uses "=" exclusively, and supporting that syntax would
help people port TSQL-based applications.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com


From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Josh Berkus <josh(at)agliodbs(dot)com>
Cc: Steve Prentice <prentice(at)cisco(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-21 19:05:33
Message-ID: 162867790905211205i76833f8coa9361ee3df9380e2@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2009/5/21 Josh Berkus <josh(at)agliodbs(dot)com>:
>
>> The patch adds the "AS" keyword to the plpgsql grammar and doesn't
>> assign an expression parameter to the sql construct if the scalar
>> follows the AS keyword.
>
> Would it be possible to also support "=" as well as "as"?  I believe that
> SQL Server uses "=" exclusively, and supporting that syntax would help
> people port TSQL-based applications.
>

No, it not possible, or not simply . This is ambiguous. a = 10 should
be normal boolean expression. I didn't use T-SQL many years, but I
thing, so in T-SQL this problem is solved with variables prefixes @.

regards
Pavel Stehule

> --
> Josh Berkus
> PostgreSQL Experts Inc.
> www.pgexperts.com
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Josh Berkus <josh(at)agliodbs(dot)com>
Cc: Steve Prentice <prentice(at)cisco(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-21 19:06:19
Message-ID: 162867790905211206s1d96379br706fb5382ce7c6ab@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2009/5/21 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> 2009/5/21 Josh Berkus <josh(at)agliodbs(dot)com>:
>>
>>> The patch adds the "AS" keyword to the plpgsql grammar and doesn't
>>> assign an expression parameter to the sql construct if the scalar
>>> follows the AS keyword.
>>
>> Would it be possible to also support "=" as well as "as"?  I believe that
>> SQL Server uses "=" exclusively, and supporting that syntax would help
>> people port TSQL-based applications.
>>
>
> No, it not possible, or not simply . This is ambiguous. a = 10 should
> be normal boolean expression. I didn't use T-SQL many years, but I
> thing, so in T-SQL this problem is solved with variables prefixes @.

there are not technical problem - there are semantic problem

Pavel

>
> regards
> Pavel Stehule
>
>> --
>> Josh Berkus
>> PostgreSQL Experts Inc.
>> www.pgexperts.com
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>


From: Steve Prentice <prentice(at)cisco(dot)com>
To: Josh Berkus <josh(at)agliodbs(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-21 19:25:51
Message-ID: A29A66E8-88B2-4D50-9DB9-12BB67429723@cisco.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"=" is more common than "as" for sure, but this has been talked about
before starting at about this message:

http://archives.postgresql.org/message-id/14319.1228833321@sss.pgh.pa.us

-Steve

On May 21, 2009, at 11:51 AM, Josh Berkus wrote:

> The patch adds the "AS" keyword to the plpgsql grammar and doesn't
> assign an expression parameter to the sql construct if the scalar
> follows the AS keyword.

Would it be possible to also support "=" as well as "as"? I believe
that SQL Server uses "=" exclusively, and supporting that syntax would
help people port TSQL-based applications.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com


From: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
To: Josh Berkus <josh(at)agliodbs(dot)com>
Cc: Steve Prentice <prentice(at)cisco(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-21 22:44:08
Message-ID: 20090521224408.GA3488@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Josh Berkus wrote:
>
>> The patch adds the "AS" keyword to the plpgsql grammar and doesn't
>> assign an expression parameter to the sql construct if the scalar
>> follows the AS keyword.
>
> Would it be possible to also support "=" as well as "as"? I believe
> that SQL Server uses "=" exclusively, and supporting that syntax would
> help people port TSQL-based applications.

Would this be best served by implementing PL/TSQL?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


From: Josh Berkus <josh(at)agliodbs(dot)com>
To: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
Cc: Steve Prentice <prentice(at)cisco(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-22 14:40:32
Message-ID: 4A16B960.3020801@agliodbs.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 5/21/09 6:44 PM, Alvaro Herrera wrote:
> Josh Berkus wrote:
>>> The patch adds the "AS" keyword to the plpgsql grammar and doesn't
>>> assign an expression parameter to the sql construct if the scalar
>>> follows the AS keyword.
>> Would it be possible to also support "=" as well as "as"? I believe
>> that SQL Server uses "=" exclusively, and supporting that syntax would
>> help people port TSQL-based applications.
>
> Would this be best served by implementing PL/TSQL?

Passing variables takes place *outside* of the PL.

--
Josh Berkus
PostgreSQL Experts Inc.
www.pgexperts.com


From: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
To: Josh Berkus <josh(at)agliodbs(dot)com>
Cc: Steve Prentice <prentice(at)cisco(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-22 15:28:47
Message-ID: 20090522152847.GA4466@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Josh Berkus wrote:
> On 5/21/09 6:44 PM, Alvaro Herrera wrote:
>> Josh Berkus wrote:
>>>> The patch adds the "AS" keyword to the plpgsql grammar and doesn't
>>>> assign an expression parameter to the sql construct if the scalar
>>>> follows the AS keyword.
>>> Would it be possible to also support "=" as well as "as"? I believe
>>> that SQL Server uses "=" exclusively, and supporting that syntax would
>>> help people port TSQL-based applications.
>>
>> Would this be best served by implementing PL/TSQL?
>
> Passing variables takes place *outside* of the PL.

Hmm, sure, but I was actually thinking on a higher level ... I mean I've always
thought that there's so much effort directed at porting procedures from TSQL to
plpgsql, but it would be better to just write a dedicated PL. (Of course,
the set of people who can actually write such a PL is a lot smaller than those
who can write plpgsql).

I mean if all you had to do was change = to "as" to make your TSQL procedures
supported, you'd be a lot happier than if that was the only work you could
_save_.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Steve Prentice <prentice(at)cisco(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v2
Date: 2009-05-22 19:41:00
Message-ID: 162867790905221241x6fbd789cu82ffc3456f3f371c@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello

2009/5/21 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Steve Prentice <prentice(at)cisco(dot)com> writes:
>> This patch is intended to supplement Pavel's patch for named and mixed
>> notation support in 8.5. This patch makes it so a plpgsql function can
>> call another function with the same parameter names using the named
>> parameters notation.
>
> Well, plpgsql's parsing has always been a kluge, but I think this is
> really taking the kluge level a step too far.  It's only because AS
> is used in so few contexts that this can even pretend to work --- but
> there are still an awful lot of contexts where AS is used, and will
> likely be more in the future.  So I think it's pretty un-future-proof;
> and it certainly won't scale to any other contexts where we might wish
> that plpsql variables don't get substituted.
>
> It's probably time to bite the bullet and redo the parser as has been
> suggested in the past, ie fix things so that the main parser is used.
> Ideally I'd like to switch the name resolution priority to be more
> Oracle-like, but even if we don't do that it would be a great
> improvement to have actual syntactic knowledge behind the lookups.

I have fast hack, that is based on callback function from
transformExpr - it replace unknown ColumnRef node to Param node. It
works well, but I have two notes:

a) when we use main parser for identification, then valid_sql flag has
different sense. We have to valid all SQL statements - so we have to
change some logic or we have to develop levels of validations. This
have one big advantage - we are able to do complete validation.
Disadvantage - check is necessary before every first run.

b) Change priority (sql identifiers >> plpgsql identifiers) will have
two impacts:

** buggy applications will have different behave
** some an table's alters should change function's behave - so minimum
is reset plpgsql cache.

postgres=# create or replace function test(a int) returns int as $$
declare x int; y int; begin return a+20 as a; end; $$ language
plpgsql;
CREATE FUNCTION
Time: 2,485 ms
postgres=# select test(20);
test
------
40
(1 row)

Time: 1,473 ms
postgres=# create or replace function test(a int) returns int as $$
declare x int; y int; begin return a+20 as a from a; end; $$ language
plpgsql;
ERROR: relation "a" does not exist
LINE 1: SELECT a+20 as a from a
^ --< pointer is correct, look on it
some non proportional font
QUERY: SELECT a+20 as a from a
CONTEXT: SQL statement in PL/PgSQL function "test" near line 1

Attached patch is VERY UGLY, but it should to show possible direction.
I thing, so some similar should by in 8.5

??? Notes, Comments

Regards
Pavel Stehule

>
> Just for the record, you'd have to put the same kluge into the T_RECORD
> and T_ROW cases if we wanted to do it like this.
>
>                        regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Attachment Content-Type Size
mainparser.diff text/x-patch 7.5 KB

From: Bernd Helmle <mailings(at)oopsware(dot)de>
To: Steve Prentice <prentice(at)cisco(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-07-17 18:56:12
Message-ID: 33ED077475A15AD3FAB0038B@teje
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

--On Donnerstag, Mai 21, 2009 11:46:24 -0700 Steve Prentice
<prentice(at)cisco(dot)com> wrote:

>> Just for the record, you'd have to put the same kluge into the
>> T_RECORD
>> and T_ROW cases if we wanted to do it like this.
>
> Patch updated.

Steve,

it seems there's something broken, patch complains about a broken format.
Can you please provide a new diff file?

Thanks

Bernd


From: Steve Prentice <prentice(at)cisco(dot)com>
To: Bernd Helmle <mailings(at)oopsware(dot)de>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-07-17 22:57:00
Message-ID: C411D0D1-6784-4B97-B1D3-06EDADFCC9FE@cisco.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Jul 17, 2009, at 11:56 AM, Bernd Helmle wrote:
> it seems there's something broken, patch complains about a broken
> format. Can you please provide a new diff file?

Sorry about that--probably got messed up as I pasted it into the
message. I've attached the patch this time.

Attachment Content-Type Size
plpgsql_keyword_as.patch application/octet-stream 3.2 KB

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Steve Prentice <prentice(at)cisco(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-09-14 02:05:00
Message-ID: 603c8f070909131905l2b33a3afs75927c40eaed2fe7@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, May 21, 2009 at 2:46 PM, Steve Prentice <prentice(at)cisco(dot)com> wrote:
> On May 21, 2009, at 10:52 AM, Tom Lane wrote:
>>
>> It's probably time to bite the bullet and redo the parser as has been
>> suggested in the past, ie fix things so that the main parser is used.
>> Ideally I'd like to switch the name resolution priority to be more
>> Oracle-like, but even if we don't do that it would be a great
>> improvement to have actual syntactic knowledge behind the lookups.
>
> That kind of refactoring is beyond my experience-level with the code, but I
> can't say I disagree with your analysis.
>
>> Just for the record, you'd have to put the same kluge into the T_RECORD
>> and T_ROW cases if we wanted to do it like this.
>
> Patch updated.

I played around a bit with the latest version of this patch tonight,
but I'm replying to this previous version for the sake of being able
to quote more of the relevant discussion.

First, I applied this patch, which resulted in a successful compile,
but PL/pgsql wouldn't load. After scratching my head for a minute, I
recalled that this was supposed to be dependent on named and mixed
notation, so I applied both patches, which resulted in a failed
compile. Further experimentation revealed that named and mixed
notation alone also lead to a failed compile. I replied to the
named/mixed notation thread so hopefully Pavel will fix whatever the
problem is with that patch.

However... even assuming I can get this to work at all, it seems like
it's only going to help in a pretty limited range of cases. Since
this is just looking for occurrences of "AS", it has a chance of
working (of course I can't test at the moment) for something like
this:

select foo as bar from generate_series(1,10) foo;

...but I think it will certainly fail for something like this:

select foo bar from generate_series(1,10) foo;

As much as I'm annoyed by the stupidity of PL/pgsql in this regard
(and I really am - I use it constantly and this is a real pain in the
neck), I think it makes more sense to wait for a more comprehensive
solution. Also, besides the fact that this doesn't (and can't) handle
all cases, as Tom points out, this would create a real possibility
that some future use of the word AS could cause breakage at a
distance.

So, I guess I'm sadly left feeling that we should probably reject this
patch. Anyone want to argue otherwise?

...Robert


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Steve Prentice <prentice(at)cisco(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-09-14 02:18:53
Message-ID: 29450.1252894733@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> So, I guess I'm sadly left feeling that we should probably reject this
> patch. Anyone want to argue otherwise?

+1. I'm really hoping to get something done about the plpgsql parsing
situation before 8.5 is out, so this should be a dead end anyway.

regards, tom lane


From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Steve Prentice <prentice(at)cisco(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-09-14 05:22:19
Message-ID: 162867790909132222i2789fc83ye2802e3cb20f5e1e@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2009/9/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> So, I guess I'm sadly left feeling that we should probably reject this
>> patch.  Anyone want to argue otherwise?
>
> +1.  I'm really hoping to get something done about the plpgsql parsing
> situation before 8.5 is out, so this should be a dead end anyway.
>

I have a WIP patch for integration main SQL parser to plpgsql. I'll
send it to this weekend.

regards
Pavel Stehule

>                        regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


From: Steve Prentice <prentice(at)cisco(dot)com>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-09-14 15:02:31
Message-ID: 6B1D3BCA-1D65-42A0-9696-38B8FC5C7DD5@cisco.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sep 13, 2009, at 10:22 PM, Pavel Stehule wrote:

> 2009/9/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> So, I guess I'm sadly left feeling that we should probably reject
>>> this
>>> patch. Anyone want to argue otherwise?
>>
>> +1. I'm really hoping to get something done about the plpgsql
>> parsing
>> situation before 8.5 is out, so this should be a dead end anyway.
>>
>
> I have a WIP patch for integration main SQL parser to plpgsql. I'll
> send it to this weekend.

I certainly don't mind the patch getting rejected and agree that
refactoring the plpgsql parser is probably the best approach to this
issue. However, I think it would be more than a little strange to ship
the named notation feature without a solution for this problem. For
reference, the problem is that the function below causes a compile
error because of the way plpgsql blindly does variable replacement:

create function fun1(pDisplayName text) returns void as $$
begin
perform fun2(pDisplayName as pDisplayName);
-- Above line compiles as:
-- SELECT fun2( $1 as $1 )
end
$$ language plpgsql;

-Steve


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Steve Prentice <prentice(at)cisco(dot)com>
Cc: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-09-14 15:21:06
Message-ID: 603c8f070909140821k3ee3a26by84a9c0ea8529a1aa@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Sep 14, 2009 at 11:02 AM, Steve Prentice <prentice(at)cisco(dot)com> wrote:
> On Sep 13, 2009, at 10:22 PM, Pavel Stehule wrote:
>
>> 2009/9/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>>
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>
>>>> So, I guess I'm sadly left feeling that we should probably reject this
>>>> patch.  Anyone want to argue otherwise?
>>>
>>> +1.  I'm really hoping to get something done about the plpgsql parsing
>>> situation before 8.5 is out, so this should be a dead end anyway.
>>>
>>
>> I have a WIP patch for integration main SQL parser to plpgsql. I'll
>> send it to this weekend.
>
> I certainly don't mind the patch getting rejected and agree that refactoring
> the plpgsql parser is probably the best approach to this issue. However, I
> think it would be more than a little strange to ship the named notation
> feature without a solution for this problem. For reference, the problem is
> that the function below causes a compile error because of the way plpgsql
> blindly does variable replacement:
>
> create function fun1(pDisplayName text) returns void as $$
> begin
>    perform fun2(pDisplayName as pDisplayName);
> -- Above line compiles as:
> --  SELECT  fun2( $1  as  $1 )
> end
> $$ language plpgsql;

Yeah but we already have this problem. Right now, it typically
happens because of some statement of the form SELECT ... AS ...; this
just adds one more case where it can happen, and I doubt it's any more
common than the case we already struggle with.

But at any rate Tom is planning a fix for 8.5, so I don't think
there's any need to get excited just yet. If Tom doesn't get his
stuff finished by January, we can revisit the issue then.

...Robert


From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Steve Prentice <prentice(at)cisco(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-09-14 15:56:56
Message-ID: 162867790909140856h1edde635v174f0d769bdfd990@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2009/9/14 Steve Prentice <prentice(at)cisco(dot)com>:
> On Sep 13, 2009, at 10:22 PM, Pavel Stehule wrote:
>
>> 2009/9/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>>
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>
>>>> So, I guess I'm sadly left feeling that we should probably reject this
>>>> patch.  Anyone want to argue otherwise?
>>>
>>> +1.  I'm really hoping to get something done about the plpgsql parsing
>>> situation before 8.5 is out, so this should be a dead end anyway.
>>>
>>
>> I have a WIP patch for integration main SQL parser to plpgsql. I'll
>> send it to this weekend.
>
> I certainly don't mind the patch getting rejected and agree that refactoring
> the plpgsql parser is probably the best approach to this issue. However, I
> think it would be more than a little strange to ship the named notation
> feature without a solution for this problem. For reference, the problem is
> that the function below causes a compile error because of the way plpgsql
> blindly does variable replacement:
>
> create function fun1(pDisplayName text) returns void as $$
> begin
>    perform fun2(pDisplayName as pDisplayName);
> -- Above line compiles as:
> --  SELECT  fun2( $1  as  $1 )
> end
> $$ language plpgsql;
>

I am sure, so this this will be solved in next commitfest. This
problem is related only to plpgsql. Other PL languages are well,
because doesn't try to emulate SQL parser.

Pavel

> -Steve
>


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: Steve Prentice <prentice(at)cisco(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] 8.5 plpgsql change for named notation: treat word following AS keyword as label v3
Date: 2009-09-14 16:10:33
Message-ID: 603c8f070909140910k7ec1d6fat5d9ff6862e682d9b@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Sep 14, 2009 at 11:56 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2009/9/14 Steve Prentice <prentice(at)cisco(dot)com>:
>> On Sep 13, 2009, at 10:22 PM, Pavel Stehule wrote:
>>
>>> 2009/9/14 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>>>
>>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>>
>>>>> So, I guess I'm sadly left feeling that we should probably reject this
>>>>> patch.  Anyone want to argue otherwise?
>>>>
>>>> +1.  I'm really hoping to get something done about the plpgsql parsing
>>>> situation before 8.5 is out, so this should be a dead end anyway.
>>>>
>>>
>>> I have a WIP patch for integration main SQL parser to plpgsql. I'll
>>> send it to this weekend.
>>
>> I certainly don't mind the patch getting rejected and agree that refactoring
>> the plpgsql parser is probably the best approach to this issue. However, I
>> think it would be more than a little strange to ship the named notation
>> feature without a solution for this problem. For reference, the problem is
>> that the function below causes a compile error because of the way plpgsql
>> blindly does variable replacement:
>>
>> create function fun1(pDisplayName text) returns void as $$
>> begin
>>    perform fun2(pDisplayName as pDisplayName);
>> -- Above line compiles as:
>> --  SELECT  fun2( $1  as  $1 )
>> end
>> $$ language plpgsql;
>>
>
> I am sure, so this this will be solved in next commitfest. This
> problem is related only to plpgsql. Other PL languages are well,
> because doesn't try to emulate SQL parser.

And the emphasis here is on "try".

...Robert