pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedMissing semicolumn in anonymous plpgsql block does not raise syntax error
13+ messages / 5 participants
[nested] [flat]
* Missing semicolumn in anonymous plpgsql block does not raise syntax error
@ 2024-06-02 11:24 Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Mor Lehr @ 2024-06-02 11:24 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi,
I would like to report a potential bug in postgres 15.4, also reproduced on
15.6.
*The exact sequence of steps:*
Connect to a postgres 15.4 database and run the following statements:
CREATE TABLE foo3(id serial PRIMARY key, txt text);
INSERT INTO foo3 (txt) VALUES ('aaa'),('bbb');
DO $$
DECLARE
l_cnt int;
BEGIN
l_cnt := 1
DELETE FROM foo3 WHERE id=1;
END; $$;
*The output you got:*
1. The script passes (no error message) even though there's a missing
semicolon (;) after "l_cnt := 1"
2. The script doesn't actually delete the record from foo3
This caused us a production issue where we thought changes were applied
(script passed successfully) but changes weren't actually applied.
If I move the line "l_cnt := 1" to after the DELETE statement like so:
DO $$
DECLARE
l_cnt int;
BEGIN
DELETE FROM foo3 WHERE id=1;
l_cnt := 1
END; $$;
I get the error - as expected:
SQL Error [42601]: ERROR: syntax error at end of input
Position: 89
Furthermore, replacing the DELETE statement with an UPDATE statement in the
original code does raise an error:
DO $$
DECLARE
l_cnt int;
BEGIN
l_cnt := 1
UPDATE foo3 SET txt='ccc' WHERE id=1;
END; $$;
SQL Error [42601]: ERROR: syntax error at or near "foo3"
Position: 62
But adding the semicolon - it works correctly with either UPDATE or DELETE.
I ran the original code using the following clients to make sure it's not a
client problem:
1. psql
2. DBeaver using standard JDBC drivers
3. Flyway using JDBC drivers
*Versions:*
PostgreSQL 15.6 (Homebrew) on x86_64-apple-darwin23.2.0, compiled by Apple
clang version 15.0.0 (clang-1500.1.0.2.5), 64-bit l - running locally on my
MacBook
PostgreSQL 15.4 on aarch64-unknown-linux-gnu, compiled by
aarch64-unknown-linux-gnu-gcc (GCC) 9.5.0, 64-bit - running on AWS RDS
Aurora
*Installed Extensions (On AWS RDS):*
oid |extname
|extowner|extnamespace|extrelocatable|extversion|extconfig |extcondition|
----------+-------------------------+--------+------------+--------------+----------+------------+------------+
16463|btree_gist | 10| 2200|true
|1.7 |NULL |NULL |
1463651797|deel_password_check_rules| 16399| 2200|false
|1.0 |NULL |NULL |
16464|fuzzystrmatch | 10| 2200|true
|1.1 |NULL |NULL |
958297705|pg_repack | 10| 2200|false
|1.4.8 |NULL |NULL |
16465|pg_stat_statements | 10| 2200|true
|1.9 |NULL |NULL |
1463506085|pg_tle | 10| 1463506084|false
|1.1.1 |{1463506117}|{""} |
16467|pg_trgm | 10| 2200|true
|1.6 |NULL |NULL |
16468|pgcrypto | 10| 2200|true
|1.3 |NULL |NULL |
14498|plpgsql | 10| 11|false
|1.0 |NULL |NULL |
16469|postgres_fdw | 10| 2200|true
|1.1 |NULL |NULL |
16470|tablefunc | 10| 2200|true
|1.0 |NULL |NULL |
16471|unaccent | 10| 2200|true
|1.1 |NULL |NULL |
16472|uuid-ossp | 10| 2200|true
|1.1 |NULL |NULL |
Please let me know what other information I can provide.
Thanks,
Mor
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
@ 2024-06-02 15:19 ` David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:26 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 22:18 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 13+ messages in thread
From: David G. Johnston @ 2024-06-02 15:19 UTC (permalink / raw)
To: Mor Lehr <mor.lehr@deel.com>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Sunday, June 2, 2024, Mor Lehr <mor.lehr@deel.com> wrote:
> Hi,
>
> I would like to report a potential bug in postgres 15.4, also reproduced
> on 15.6.
>
> *The exact sequence of steps:*
> Connect to a postgres 15.4 database and run the following statements:
>
> CREATE TABLE foo3(id serial PRIMARY key, txt text);
>
> INSERT INTO foo3 (txt) VALUES ('aaa'),('bbb');
>
> DO $$
>
> DECLARE
>
> l_cnt int;
>
> BEGIN
>
> l_cnt := 1
>
> DELETE FROM foo3 WHERE id=1;
>
> END; $$;
>
>
> *The output you got:*
>
> 1. The script passes (no error message) even though there's a missing
> semicolon (;) after "l_cnt := 1"
> 2. The script doesn't actually delete the record from foo3
>
>
>
I think you just wrote the equivalent of:
l_cnt := (select 1 as delete from foo3 where id=1);
Which is a valid query.
David J.
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
@ 2024-06-02 15:26 ` Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:31 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
1 sibling, 1 reply; 13+ messages in thread
From: Mor Lehr @ 2024-06-02 15:26 UTC (permalink / raw)
To: David G. Johnston <david.g.johnston@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Thanks for the prompt reply.
Can you please refer me to the section in the documentation that describes
this behavior?
This (automatically interperting 1 as select 1) is totally an unexpected
behavior for me.
Thanks again.
-Mor.
On Sun, Jun 2, 2024, 18:19 David G. Johnston <david.g.johnston@gmail.com>
wrote:
> On Sunday, June 2, 2024, Mor Lehr <mor.lehr@deel.com> wrote:
>
>> Hi,
>>
>> I would like to report a potential bug in postgres 15.4, also reproduced
>> on 15.6.
>>
>> *The exact sequence of steps:*
>> Connect to a postgres 15.4 database and run the following statements:
>>
>> CREATE TABLE foo3(id serial PRIMARY key, txt text);
>>
>> INSERT INTO foo3 (txt) VALUES ('aaa'),('bbb');
>>
>> DO $$
>>
>> DECLARE
>>
>> l_cnt int;
>>
>> BEGIN
>>
>> l_cnt := 1
>>
>> DELETE FROM foo3 WHERE id=1;
>>
>> END; $$;
>>
>>
>> *The output you got:*
>>
>> 1. The script passes (no error message) even though there's a missing
>> semicolon (;) after "l_cnt := 1"
>> 2. The script doesn't actually delete the record from foo3
>>
>>
>>
> I think you just wrote the equivalent of:
>
> l_cnt := (select 1 as delete from foo3 where id=1);
>
> Which is a valid query.
>
> David J.
>
>
>
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:26 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
@ 2024-06-02 15:31 ` David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:42 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
0 siblings, 1 reply; 13+ messages in thread
From: David G. Johnston @ 2024-06-02 15:31 UTC (permalink / raw)
To: Mor Lehr <mor.lehr@deel.com>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Sunday, June 2, 2024, Mor Lehr <mor.lehr@deel.com> wrote:
> Thanks for the prompt reply.
> Can you please refer me to the section in the documentation that describes
> this behavior?
> This (automatically interperting 1 as select 1) is totally an unexpected
> behavior for me.
>
https://www.postgresql.org/docs/current/plpgsql-statements.html#PLPGSQL-STATEMENTS-ASSIGNMENT
“As explained previously, the expression in such a statement is evaluated
by means of an SQL SELECT command sent to the main database engine.”
David J.
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:26 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:31 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
@ 2024-06-02 15:42 ` Mor Lehr <mor.lehr@deel.com>
2025-02-06 06:47 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Mor Lehr @ 2024-06-02 15:42 UTC (permalink / raw)
To: David G. Johnston <david.g.johnston@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Thanks for the reference.
We learn new stuff every day.
You can close the case.
Thanks, Mor
On Sun, Jun 2, 2024, 18:31 David G. Johnston <david.g.johnston@gmail.com>
wrote:
> On Sunday, June 2, 2024, Mor Lehr <mor.lehr@deel.com> wrote:
>
>> Thanks for the prompt reply.
>> Can you please refer me to the section in the documentation that
>> describes this behavior?
>> This (automatically interperting 1 as select 1) is totally an unexpected
>> behavior for me.
>>
>
>
> https://www.postgresql.org/docs/current/plpgsql-statements.html#PLPGSQL-STATEMENTS-ASSIGNMENT
>
> “As explained previously, the expression in such a statement is evaluated
> by means of an SQL SELECT command sent to the main database engine.”
>
> David J.
>
>
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:26 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:31 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:42 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
@ 2025-02-06 06:47 ` Pavel Stehule <pavel.stehule@gmail.com>
2025-02-06 10:10 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
0 siblings, 1 reply; 13+ messages in thread
From: Pavel Stehule @ 2025-02-06 06:47 UTC (permalink / raw)
To: Mor Lehr <mor.lehr@deel.com>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; pgsql-bugs@lists.postgresql.org
Hi
ne 2. 6. 2024 v 18:25 odesílatel Mor Lehr <mor.lehr@deel.com> napsal:
> Thanks for the reference.
> We learn new stuff every day.
>
> You can close the case.
> Thanks, Mor
>
>
plpgsql_check can now detect this issue
https://okbob.blogspot.com/2025/02/plpgsqlcheck-raise-warning-when-syntax.html
Regards
Pavel
> On Sun, Jun 2, 2024, 18:31 David G. Johnston <david.g.johnston@gmail.com>
> wrote:
>
>> On Sunday, June 2, 2024, Mor Lehr <mor.lehr@deel.com> wrote:
>>
>>> Thanks for the prompt reply.
>>> Can you please refer me to the section in the documentation that
>>> describes this behavior?
>>> This (automatically interperting 1 as select 1) is totally an unexpected
>>> behavior for me.
>>>
>>
>>
>> https://www.postgresql.org/docs/current/plpgsql-statements.html#PLPGSQL-STATEMENTS-ASSIGNMENT
>>
>> “As explained previously, the expression in such a statement is evaluated
>> by means of an SQL SELECT command sent to the main database engine.”
>>
>> David J.
>>
>>
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:26 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:31 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:42 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2025-02-06 06:47 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
@ 2025-02-06 10:10 ` Mor Lehr <mor.lehr@deel.com>
0 siblings, 0 replies; 13+ messages in thread
From: Mor Lehr @ 2025-02-06 10:10 UTC (permalink / raw)
To: Pavel Stehule <pavel.stehule@gmail.com>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; pgsql-bugs@lists.postgresql.org
Thanks for not letting this be forgotten :)
Unfortunately, plpgsql_check does not help me because it's not supported on
AWS.
I also saw you wrote a patch which is still pending, I will probably have
to wait for that.
Thanks again,
Mor
On Thu, Feb 6, 2025 at 7:48 AM Pavel Stehule <pavel.stehule@gmail.com>
wrote:
> Hi
>
> ne 2. 6. 2024 v 18:25 odesílatel Mor Lehr <mor.lehr@deel.com> napsal:
>
>> Thanks for the reference.
>> We learn new stuff every day.
>>
>> You can close the case.
>> Thanks, Mor
>>
>>
> plpgsql_check can now detect this issue
>
>
> https://okbob.blogspot.com/2025/02/plpgsqlcheck-raise-warning-when-syntax.html
>
> Regards
>
> Pavel
>
>
>> On Sun, Jun 2, 2024, 18:31 David G. Johnston <david.g.johnston@gmail.com>
>> wrote:
>>
>>> On Sunday, June 2, 2024, Mor Lehr <mor.lehr@deel.com> wrote:
>>>
>>>> Thanks for the prompt reply.
>>>> Can you please refer me to the section in the documentation that
>>>> describes this behavior?
>>>> This (automatically interperting 1 as select 1) is totally an
>>>> unexpected behavior for me.
>>>>
>>>
>>>
>>> https://www.postgresql.org/docs/current/plpgsql-statements.html#PLPGSQL-STATEMENTS-ASSIGNMENT
>>>
>>> “As explained previously, the expression in such a statement is
>>> evaluated by means of an SQL SELECT command sent to the main database
>>> engine.”
>>>
>>> David J.
>>>
>>>
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
@ 2024-06-02 22:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-06-03 16:45 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Erik Wienhold <ewie@ewie.name>
1 sibling, 1 reply; 13+ messages in thread
From: Tom Lane @ 2024-06-02 22:18 UTC (permalink / raw)
To: David G. Johnston <david.g.johnston@gmail.com>; +Cc: Mor Lehr <mor.lehr@deel.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I think you just wrote the equivalent of:
> l_cnt := (select 1 as delete from foo3 where id=1);
> Which is a valid query.
Still another example of the folly of letting AS be optional.
I don't suppose we can ever undo that though.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 22:18 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-06-03 16:45 ` Erik Wienhold <ewie@ewie.name>
2024-06-03 18:11 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Erik Wienhold @ 2024-06-03 16:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; Mor Lehr <mor.lehr@deel.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On 2024-06-03 00:18 +0200, Tom Lane wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
> > I think you just wrote the equivalent of:
> > l_cnt := (select 1 as delete from foo3 where id=1);
> > Which is a valid query.
>
> Still another example of the folly of letting AS be optional.
> I don't suppose we can ever undo that though.
How about inventing an opt-in strict mode (like in Perl or JavaScript)
that prevents certain footguns? For example, disallowing bare column
labels.
That could be enabled for the current session or transaction:
SET strict_parsing = { on | off };
Or just for individual routines:
CREATE PROCEDURE myproc()
SET strict_parsing = { on | off }
LANGUAGE plpgsql ...
--
Erik
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 22:18 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Tom Lane <tgl@sss.pgh.pa.us>
2024-06-03 16:45 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Erik Wienhold <ewie@ewie.name>
@ 2024-06-03 18:11 ` Pavel Stehule <pavel.stehule@gmail.com>
2024-06-04 06:39 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
0 siblings, 1 reply; 13+ messages in thread
From: Pavel Stehule @ 2024-06-03 18:11 UTC (permalink / raw)
To: Erik Wienhold <ewie@ewie.name>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; Mor Lehr <mor.lehr@deel.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
po 3. 6. 2024 v 18:46 odesílatel Erik Wienhold <ewie@ewie.name> napsal:
> On 2024-06-03 00:18 +0200, Tom Lane wrote:
> > "David G. Johnston" <david.g.johnston@gmail.com> writes:
> > > I think you just wrote the equivalent of:
> > > l_cnt := (select 1 as delete from foo3 where id=1);
> > > Which is a valid query.
> >
> > Still another example of the folly of letting AS be optional.
> > I don't suppose we can ever undo that though.
>
> How about inventing an opt-in strict mode (like in Perl or JavaScript)
> that prevents certain footguns? For example, disallowing bare column
> labels.
>
> That could be enabled for the current session or transaction:
>
> SET strict_parsing = { on | off };
>
> Or just for individual routines:
>
> CREATE PROCEDURE myproc()
> SET strict_parsing = { on | off }
> LANGUAGE plpgsql ...
>
>
Probably it is not bad idea - it can be generally useful
But I think it is better to introduce a new entry for plpgsql expressions
in gram.y.
Unfortunately it is not a compatible change. Years ago was popular to use a
pattern
a := tab.a FROM tab
instead correct
a := (SELECT tab.a FROM tab)
or
SELECT tab.a FROM tab INTO a;
Regards
Pavel
> --
> Erik
>
>
>
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 22:18 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Tom Lane <tgl@sss.pgh.pa.us>
2024-06-03 16:45 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Erik Wienhold <ewie@ewie.name>
2024-06-03 18:11 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
@ 2024-06-04 06:39 ` Mor Lehr <mor.lehr@deel.com>
2024-06-10 11:56 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Mor Lehr @ 2024-06-04 06:39 UTC (permalink / raw)
To: Pavel Stehule <pavel.stehule@gmail.com>; +Cc: Erik Wienhold <ewie@ewie.name>; Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
>
> How about inventing an opt-in strict mode
That can be useful at the session level, because we use anonymous blocks
quite often.
I assume if such a setting existed - we would have used it in the original
scenario.
Thanks again,
-Mor
On Mon, Jun 3, 2024 at 9:12 PM Pavel Stehule <pavel.stehule@gmail.com>
wrote:
>
>
> po 3. 6. 2024 v 18:46 odesílatel Erik Wienhold <ewie@ewie.name> napsal:
>
>> On 2024-06-03 00:18 +0200, Tom Lane wrote:
>> > "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> > > I think you just wrote the equivalent of:
>> > > l_cnt := (select 1 as delete from foo3 where id=1);
>> > > Which is a valid query.
>> >
>> > Still another example of the folly of letting AS be optional.
>> > I don't suppose we can ever undo that though.
>>
>> How about inventing an opt-in strict mode (like in Perl or JavaScript)
>> that prevents certain footguns? For example, disallowing bare column
>> labels.
>>
>> That could be enabled for the current session or transaction:
>>
>> SET strict_parsing = { on | off };
>>
>> Or just for individual routines:
>>
>> CREATE PROCEDURE myproc()
>> SET strict_parsing = { on | off }
>> LANGUAGE plpgsql ...
>>
>>
> Probably it is not bad idea - it can be generally useful
>
> But I think it is better to introduce a new entry for plpgsql expressions
> in gram.y.
>
> Unfortunately it is not a compatible change. Years ago was popular to use
> a pattern
>
> a := tab.a FROM tab
>
> instead correct
>
> a := (SELECT tab.a FROM tab)
>
> or
>
> SELECT tab.a FROM tab INTO a;
>
> Regards
>
> Pavel
>
>
>
>> --
>> Erik
>>
>>
>>
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 22:18 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Tom Lane <tgl@sss.pgh.pa.us>
2024-06-03 16:45 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Erik Wienhold <ewie@ewie.name>
2024-06-03 18:11 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
2024-06-04 06:39 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
@ 2024-06-10 11:56 ` Pavel Stehule <pavel.stehule@gmail.com>
2024-06-10 15:00 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Pavel Stehule @ 2024-06-10 11:56 UTC (permalink / raw)
To: Mor Lehr <mor.lehr@deel.com>; +Cc: Erik Wienhold <ewie@ewie.name>; Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Hi
út 4. 6. 2024 v 8:39 odesílatel Mor Lehr <mor.lehr@deel.com> napsal:
> How about inventing an opt-in strict mode
>
>
> That can be useful at the session level, because we use anonymous blocks
> quite often.
> I assume if such a setting existed - we would have used it in the original
> scenario.
>
> Thanks again,
> -Mor
>
> On Mon, Jun 3, 2024 at 9:12 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>
>>
>>
>> po 3. 6. 2024 v 18:46 odesílatel Erik Wienhold <ewie@ewie.name> napsal:
>>
>>> On 2024-06-03 00:18 +0200, Tom Lane wrote:
>>> > "David G. Johnston" <david.g.johnston@gmail.com> writes:
>>> > > I think you just wrote the equivalent of:
>>> > > l_cnt := (select 1 as delete from foo3 where id=1);
>>> > > Which is a valid query.
>>> >
>>> > Still another example of the folly of letting AS be optional.
>>> > I don't suppose we can ever undo that though.
>>>
>>> How about inventing an opt-in strict mode (like in Perl or JavaScript)
>>> that prevents certain footguns? For example, disallowing bare column
>>> labels.
>>>
>>> That could be enabled for the current session or transaction:
>>>
>>> SET strict_parsing = { on | off };
>>>
>>> Or just for individual routines:
>>>
>>> CREATE PROCEDURE myproc()
>>> SET strict_parsing = { on | off }
>>> LANGUAGE plpgsql ...
>>>
>>>
>> Probably it is not bad idea - it can be generally useful
>>
>> But I think it is better to introduce a new entry for plpgsql expressions
>> in gram.y.
>>
>> Unfortunately it is not a compatible change. Years ago was popular to use
>> a pattern
>>
>> a := tab.a FROM tab
>>
>> instead correct
>>
>> a := (SELECT tab.a FROM tab)
>>
>> or
>>
>> SELECT tab.a FROM tab INTO a;
>>
>> Regards
>>
>> Pavel
>>
>
I wrote an experimental patch that enforces strict syntax for PL/pgSQL
expressions. This patch is a proof concept and shows impacts of the change
(check-world tests passed)
Unfortunately it introduces break compatibility - with this patch the odd
syntax (that is undocumented) is not supported. Something like
DECLARE _x int := x FROM foo WHERE id = _arg;
should not be written in strict mode;
This patch very well fix reported issue:
(2024-06-10 12:06:43) postgres=# CREATE TABLE foo3(id serial PRIMARY key,
txt text);
CREATE TABLE
(2024-06-10 12:07:13) postgres=# INSERT INTO foo3 (txt) VALUES
('aaa'),('bbb');
INSERT 0 2
(2024-06-10 12:07:33) postgres=# DO $$
DECLARE
l_cnt int;
BEGIN
l_cnt := 1
DELETE FROM foo3 WHERE id=1;
END; $$;
ERROR: syntax error at or near "DELETE"
LINE 11: DELETE FROM foo3 WHERE id=1;
^
Personally, I think it can be a strong step forward (we can use deeper
integration of plpgsql with SQL parser which was not possible before).
Because it introduces a compatibility break I don't propose to use it by
default. I see two possible ways, how this check can be used:
1. we can use plpgsql.extra_errors (
https://www.postgresql.org/docs/current/plpgsql-development-tips.html#PLPGSQL-EXTRA-CHECKS
) and introduce a check named (maybe) strict_expr_syntax. Because in this
case the warning cannot be raised, then this check cannot be used in
plpgsql.extra_warnings. I think it can work nicely.
2. if @1 will not be possible, the minimalist implementation can be based
on a new public entry to SQL parser. In this case, I can do the proposed
check at least from plpgsql_check.
Do you see any other possibilities?
This patch is just a proof concept. I didn't implement any mechanism to
switch from default mode to strict mode (I don't propose strict mode as
default) now.
I think it can increase the robustness of plpgsql, on the other hand it
introduces compatibility breaks and I understand related problems.
The change of definition of PLpgSQL_Expr has an impact on OPEN and CASE
PLpgSQL statements that use multicolumn results.
Regards
Pavel
>
>>
>>
>>> --
>>> Erik
>>>
>>>
>>>
Attachments:
plpgsql-strict-expr.patch (text/x-patch, 26.4K) download
commit 108d99d96cffe5468ec95308a34f9164d8310119
Author: okbob@github.com <pavel.stehule@gmail.com>
Date: Sat Jun 8 13:09:40 2024 +0200
use strict rules for parsing PL/pgSQL expressions
Originaly the rule PLpgSQL_Expr allows almost all SQL clauses. It was designed
to allow old undocummented syntax
var := col FROM tab;
The reason for support of this "strange" syntax was technical. The PLpgSQL parser
cannot to use SQL parser accurately (it was really primitive), and people found
this undocumented syntax. Lattery, when it was possible to do exact parsing, from
compatibility reasons, the parsing of PL/pgSQL expressions allows described syntax.
Unfortunately, with support almost all SQL clauses, the PLpgSQL can accept
really broken code like
DO $$
DECLARE
l_cnt int;
BEGIN
l_cnt := 1
DELETE FROM foo3 WHERE id=1;
END; $$;
proposed patch use strictly a_expr for PLpgSQL expressions, and solve this issue.
The change of definition of PLpgSQL_Expr has an impact on OPEN and CASE
PLpgSQL statements that use multicolumn results.
This patch has lot of impacts on regress tests - it can be zero, if the strict
syntax will be enabled by using plpgsql.extra_errors instead. Now, the changes
of regress tests can be interesting for visibility of impact of strict mode.
diff --git a/contrib/basic_archive/expected/basic_archive.out b/contrib/basic_archive/expected/basic_archive.out
index 0015053e0f..280ff3e022 100644
--- a/contrib/basic_archive/expected/basic_archive.out
+++ b/contrib/basic_archive/expected/basic_archive.out
@@ -11,8 +11,8 @@ DECLARE
loops int := 0;
BEGIN
LOOP
- archived := count(*) > 0 FROM pg_ls_dir('.', false, false) a
- WHERE a ~ '^[0-9A-F]{24}$';
+ archived := (SELECT count(*) > 0 FROM pg_ls_dir('.', false, false) a
+ WHERE a ~ '^[0-9A-F]{24}$');
IF archived OR loops > 120 * 10 THEN EXIT; END IF;
PERFORM pg_sleep(0.1);
loops := loops + 1;
diff --git a/contrib/basic_archive/sql/basic_archive.sql b/contrib/basic_archive/sql/basic_archive.sql
index 14e236d57a..2c127a821f 100644
--- a/contrib/basic_archive/sql/basic_archive.sql
+++ b/contrib/basic_archive/sql/basic_archive.sql
@@ -7,8 +7,8 @@ DECLARE
loops int := 0;
BEGIN
LOOP
- archived := count(*) > 0 FROM pg_ls_dir('.', false, false) a
- WHERE a ~ '^[0-9A-F]{24}$';
+ archived := (SELECT count(*) > 0 FROM pg_ls_dir('.', false, false) a
+ WHERE a ~ '^[0-9A-F]{24}$');
IF archived OR loops > 120 * 10 THEN EXIT; END IF;
PERFORM pg_sleep(0.1);
loops := loops + 1;
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index e516c0a67c..40d98da628 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2983,6 +2983,10 @@ _SPI_error_callback(void *arg)
case RAW_PARSE_PLPGSQL_ASSIGN3:
errcontext("PL/pgSQL assignment \"%s\"", query);
break;
+ case RAW_PARSE_PLPGSQL_EXPR_LIST:
+ case RAW_PARSE_PLPGSQL_EXPR_WITH_LABEL_LIST:
+ errcontext("PL/pgSQL stetement OPEN \"%s\"", query);
+ break;
default:
errcontext("SQL statement \"%s\"", query);
break;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 4d582950b7..e5409bb7f2 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -65,7 +65,6 @@
#include "utils/numeric.h"
#include "utils/xml.h"
-
/*
* Location tracking support --- simpler than bison's default, since we only
* want to track the start position not the end position of each nonterminal.
@@ -323,7 +322,11 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <node> select_no_parens select_with_parens select_clause
simple_select values_clause
- PLpgSQL_Expr PLAssignStmt
+ PLpgSQL_Expr PLAssignStmt PLpgSQL_Expr_list
+ PLpgSQL_Expr_with_label_list
+
+%type <target> plpgsql_unnamed_target_el
+%type <list> plpgsql_unnamed_target_list
%type <str> opt_single_name
%type <list> opt_qualified_name
@@ -439,7 +442,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
name_list role_list from_clause from_list opt_array_bounds
qualified_name_list any_name any_name_list type_name_list
any_operator expr_list attrs
- distinct_clause opt_distinct_clause
+ distinct_clause
target_list opt_target_list insert_column_list set_target_list
merge_values_clause
set_clause_list set_clause
@@ -827,7 +830,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%token MODE_PLPGSQL_ASSIGN1
%token MODE_PLPGSQL_ASSIGN2
%token MODE_PLPGSQL_ASSIGN3
-
+%token MODE_PLPGSQL_EXPR_LIST
+%token MODE_PLPGSQL_EXPR_WITH_LABEL_LIST
/* Precedence: lowest to highest */
%left UNION EXCEPT
@@ -958,6 +962,16 @@ parse_toplevel:
pg_yyget_extra(yyscanner)->parsetree =
list_make1(makeRawStmt((Node *) n, 0));
}
+ | MODE_PLPGSQL_EXPR_LIST PLpgSQL_Expr_list
+ {
+ pg_yyget_extra(yyscanner)->parsetree =
+ list_make1(makeRawStmt($2, 0));
+ }
+ | MODE_PLPGSQL_EXPR_WITH_LABEL_LIST PLpgSQL_Expr_with_label_list
+ {
+ pg_yyget_extra(yyscanner)->parsetree =
+ list_make1(makeRawStmt($2, 0));
+ }
;
/*
@@ -13118,11 +13132,6 @@ opt_all_clause:
| /*EMPTY*/
;
-opt_distinct_clause:
- distinct_clause { $$ = $1; }
- | opt_all_clause { $$ = NIL; }
- ;
-
opt_sort_clause:
sort_clause { $$ = $1; }
| /*EMPTY*/ { $$ = NIL; }
@@ -17429,7 +17438,6 @@ role_list: RoleSpec
{ $$ = lappend($1, $3); }
;
-
/*****************************************************************************
*
* PL/pgSQL extensions
@@ -17439,34 +17447,61 @@ role_list: RoleSpec
* Therefore the returned struct is a SelectStmt.
*****************************************************************************/
-PLpgSQL_Expr: opt_distinct_clause opt_target_list
- from_clause where_clause
- group_clause having_clause window_clause
- opt_sort_clause opt_select_limit opt_for_locking_clause
+PLpgSQL_Expr: a_expr
{
SelectStmt *n = makeNode(SelectStmt);
+ ResTarget *rt;
+
+ rt = makeNode(ResTarget);
+ rt->name = NULL;
+ rt->indirection = NIL;
+ rt->val = (Node *) $1;
+ rt->location = @1;
+
+ n->targetList = list_make1((Node *) rt);
+
+ $$ = (Node *) n;
+ }
+ ;
+
+plpgsql_unnamed_target_el: a_expr
+ {
+ $$ = makeNode(ResTarget);
+ $$->name = NULL;
+ $$->indirection = NIL;
+ $$->val = (Node *) $1;
+ $$->location = @1;
+ }
+ ;
+
+plpgsql_unnamed_target_list:
+ plpgsql_unnamed_target_el
+ {
+ $$ = list_make1($1);
+ }
+ | plpgsql_unnamed_target_list ',' plpgsql_unnamed_target_el
+ {
+ $$ = lappend($1, $3);
+ }
+ ;
+
+PLpgSQL_Expr_list: plpgsql_unnamed_target_list
+ {
+ SelectStmt *n = makeNode(SelectStmt);
+
+ n->targetList = $1;
+
+ $$ = (Node *) n;
+
+ }
+ ;
+
+PLpgSQL_Expr_with_label_list: target_list
+ {
+ SelectStmt *n = makeNode(SelectStmt);
+
+ n->targetList = $1;
- n->distinctClause = $1;
- n->targetList = $2;
- n->fromClause = $3;
- n->whereClause = $4;
- n->groupClause = ($5)->list;
- n->groupDistinct = ($5)->distinct;
- n->havingClause = $6;
- n->windowClause = $7;
- n->sortClause = $8;
- if ($9)
- {
- n->limitOffset = $9->limitOffset;
- n->limitCount = $9->limitCount;
- if (!n->sortClause &&
- $9->limitOption == LIMIT_OPTION_WITH_TIES)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("WITH TIES cannot be specified without ORDER BY clause")));
- n->limitOption = $9->limitOption;
- }
- n->lockingClause = $10;
$$ = (Node *) n;
}
;
diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c
index 118488c3f3..5fc94695fb 100644
--- a/src/backend/parser/parser.c
+++ b/src/backend/parser/parser.c
@@ -62,6 +62,8 @@ raw_parser(const char *str, RawParseMode mode)
[RAW_PARSE_PLPGSQL_ASSIGN1] = MODE_PLPGSQL_ASSIGN1,
[RAW_PARSE_PLPGSQL_ASSIGN2] = MODE_PLPGSQL_ASSIGN2,
[RAW_PARSE_PLPGSQL_ASSIGN3] = MODE_PLPGSQL_ASSIGN3,
+ [RAW_PARSE_PLPGSQL_EXPR_LIST] = MODE_PLPGSQL_EXPR_LIST,
+ [RAW_PARSE_PLPGSQL_EXPR_WITH_LABEL_LIST] = MODE_PLPGSQL_EXPR_WITH_LABEL_LIST,
};
yyextra.have_lookahead = true;
diff --git a/src/include/parser/parser.h b/src/include/parser/parser.h
index be184ec506..890b9451e2 100644
--- a/src/include/parser/parser.h
+++ b/src/include/parser/parser.h
@@ -33,6 +33,12 @@
* RAW_PARSE_PLPGSQL_ASSIGNn: parse a PL/pgSQL assignment statement,
* and return a one-element List containing a RawStmt node. "n"
* gives the number of dotted names comprising the target ColumnRef.
+ *
+ * RAW_PARSE_PLPGSQL_EXPR_LIST is used by PLpgSQL cursors when we detect
+ * no named argument is used.
+ *
+ * RAW_PARSE_PLPGSQL_EXPR_WITH_LABEL_LIST is used for cursor's argumet
+ * when named arguments are used.
*/
typedef enum
{
@@ -42,6 +48,8 @@ typedef enum
RAW_PARSE_PLPGSQL_ASSIGN1,
RAW_PARSE_PLPGSQL_ASSIGN2,
RAW_PARSE_PLPGSQL_ASSIGN3,
+ RAW_PARSE_PLPGSQL_EXPR_LIST,
+ RAW_PARSE_PLPGSQL_EXPR_WITH_LABEL_LIST
} RawParseMode;
/* Values for the backslash_quote GUC */
diff --git a/src/interfaces/ecpg/preproc/parse.pl b/src/interfaces/ecpg/preproc/parse.pl
index fe8d3e5178..9c96d6e017 100644
--- a/src/interfaces/ecpg/preproc/parse.pl
+++ b/src/interfaces/ecpg/preproc/parse.pl
@@ -89,7 +89,11 @@ my %replace_types = (
'PLpgSQL_Expr' => 'ignore',
'PLAssignStmt' => 'ignore',
'plassign_target' => 'ignore',
- 'plassign_equals' => 'ignore',);
+ 'plassign_equals' => 'ignore',
+ 'PLpgSQL_Expr_list' => 'ignore',
+ 'PLpgSQL_Expr_with_label_list' => 'ignore',
+ 'plpgsql_unnamed_target_el' => 'ignore',
+ 'plpgsql_unnamed_target_list' => 'ignore');
# these replace_line commands excise certain keywords from the core keyword
# lists. Be sure to account for these in ColLabel and related productions.
diff --git a/src/pl/plpgsql/src/expected/plpgsql_array.out b/src/pl/plpgsql/src/expected/plpgsql_array.out
index ad60e0e8be..6819c0435f 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_array.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_array.out
@@ -50,34 +50,38 @@ do $$ declare a quadarray;
begin a.c1[1].i := 11; raise notice 'a = %, a.c1[1].i = %', a, a.c1[1].i; end$$;
NOTICE: a = ("{""(,11)""}",), a.c1[1].i = 11
do $$ declare a int[];
-begin a := array_agg(x) from (values(1),(2),(3)) v(x); raise notice 'a = %', a; end$$;
+begin a := (select array_agg(x) from (values(1),(2),(3)) v(x)); raise notice 'a = %', a; end$$;
NOTICE: a = {1,2,3}
create temp table onecol as select array[1,2] as f1;
do $$ declare a int[];
-begin a := f1 from onecol; raise notice 'a = %', a; end$$;
+begin a := (select f1 from onecol); raise notice 'a = %', a; end$$;
NOTICE: a = {1,2}
do $$ declare a int[];
-begin a := * from onecol for update; raise notice 'a = %', a; end$$;
+begin a := (select * from onecol for update); raise notice 'a = %', a; end$$;
NOTICE: a = {1,2}
-- error cases:
do $$ declare a int[];
-begin a := from onecol; raise notice 'a = %', a; end$$;
-ERROR: assignment source returned 0 columns
-CONTEXT: PL/pgSQL assignment "a := from onecol"
-PL/pgSQL function inline_code_block line 2 at assignment
+begin a := (select from onecol); raise notice 'a = %', a; end$$;
+ERROR: subquery must return only one column
+LINE 1: a := (select from onecol)
+ ^
+QUERY: a := (select from onecol)
+CONTEXT: PL/pgSQL function inline_code_block line 2 at assignment
do $$ declare a int[];
-begin a := f1, f1 from onecol; raise notice 'a = %', a; end$$;
-ERROR: assignment source returned 2 columns
-CONTEXT: PL/pgSQL assignment "a := f1, f1 from onecol"
-PL/pgSQL function inline_code_block line 2 at assignment
+begin a := (select f1, f1 from onecol); raise notice 'a = %', a; end$$;
+ERROR: subquery must return only one column
+LINE 1: a := (select f1, f1 from onecol)
+ ^
+QUERY: a := (select f1, f1 from onecol)
+CONTEXT: PL/pgSQL function inline_code_block line 2 at assignment
insert into onecol values(array[11]);
do $$ declare a int[];
-begin a := f1 from onecol; raise notice 'a = %', a; end$$;
-ERROR: query returned more than one row
-CONTEXT: query: a := f1 from onecol
+begin a := (select f1 from onecol); raise notice 'a = %', a; end$$;
+ERROR: more than one row returned by a subquery used as an expression
+CONTEXT: PL/pgSQL assignment "a := (select f1 from onecol)"
PL/pgSQL function inline_code_block line 2 at assignment
do $$ declare a int[];
-begin a := f1 from onecol limit 1; raise notice 'a = %', a; end$$;
+begin a := (select f1 from onecol limit 1); raise notice 'a = %', a; end$$;
NOTICE: a = {1,2}
do $$ declare a real;
begin a[1] := 2; raise notice 'a = %', a; end$$;
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index a29d2dfacd..2c36bc6800 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -70,6 +70,8 @@ static PLpgSQL_expr *read_sql_construct(int until,
int *endtoken);
static PLpgSQL_expr *read_sql_expression(int until,
const char *expected);
+static PLpgSQL_expr *read_sql_expressions(int until,
+ const char *expected);
static PLpgSQL_expr *read_sql_expression2(int until, int until2,
const char *expected,
int *endtoken);
@@ -176,7 +178,7 @@ static void check_raise_parameters(PLpgSQL_stmt_raise *stmt);
%type <nsitem> decl_aliasitem
%type <expr> expr_until_semi
-%type <expr> expr_until_then expr_until_loop opt_expr_until_when
+%type <expr> expr_until_then expr_until_loop opt_expr_until_when exprs_until_then
%type <expr> opt_exitcond
%type <var> cursor_variable
@@ -1239,7 +1241,7 @@ case_when_list : case_when_list case_when
}
;
-case_when : K_WHEN expr_until_then proc_sect
+case_when : K_WHEN exprs_until_then proc_sect
{
PLpgSQL_case_when *new = palloc(sizeof(PLpgSQL_case_when));
@@ -1250,6 +1252,10 @@ case_when : K_WHEN expr_until_then proc_sect
}
;
+exprs_until_then :
+ { $$ = read_sql_expressions(K_THEN, "THEN"); }
+ ;
+
opt_case_else :
{
$$ = NIL;
@@ -2641,6 +2647,15 @@ read_sql_expression(int until, const char *expected)
true, true, NULL, NULL);
}
+/* Convenience routine to read an expression with one possible terminator */
+static PLpgSQL_expr *
+read_sql_expressions(int until, const char *expected)
+{
+ return read_sql_construct(until, 0, 0, expected,
+ RAW_PARSE_PLPGSQL_EXPR_LIST,
+ true, true, NULL, NULL);
+}
+
/* Convenience routine to read an expression with two possible terminators */
static PLpgSQL_expr *
read_sql_expression2(int until, int until2, const char *expected,
@@ -3977,7 +3992,7 @@ read_cursor_args(PLpgSQL_var *cursor, int until)
expr = palloc0(sizeof(PLpgSQL_expr));
expr->query = pstrdup(ds.data);
- expr->parseMode = RAW_PARSE_PLPGSQL_EXPR;
+ expr->parseMode = any_named ? RAW_PARSE_PLPGSQL_EXPR_WITH_LABEL_LIST : RAW_PARSE_PLPGSQL_EXPR_LIST;
expr->plan = NULL;
expr->paramnos = NULL;
expr->target_param = -1;
@@ -4151,7 +4166,7 @@ make_case(int location, PLpgSQL_expr *t_expr,
StringInfoData ds;
/* We expect to have expressions not statements */
- Assert(expr->parseMode == RAW_PARSE_PLPGSQL_EXPR);
+ Assert(expr->parseMode == RAW_PARSE_PLPGSQL_EXPR_LIST);
/* Do the string hacking */
initStringInfo(&ds);
diff --git a/src/pl/plpgsql/src/sql/plpgsql_array.sql b/src/pl/plpgsql/src/sql/plpgsql_array.sql
index 4b9ff51594..699713696d 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_array.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_array.sql
@@ -46,31 +46,31 @@ do $$ declare a quadarray;
begin a.c1[1].i := 11; raise notice 'a = %, a.c1[1].i = %', a, a.c1[1].i; end$$;
do $$ declare a int[];
-begin a := array_agg(x) from (values(1),(2),(3)) v(x); raise notice 'a = %', a; end$$;
+begin a := (select array_agg(x) from (values(1),(2),(3)) v(x)); raise notice 'a = %', a; end$$;
create temp table onecol as select array[1,2] as f1;
do $$ declare a int[];
-begin a := f1 from onecol; raise notice 'a = %', a; end$$;
+begin a := (select f1 from onecol); raise notice 'a = %', a; end$$;
do $$ declare a int[];
-begin a := * from onecol for update; raise notice 'a = %', a; end$$;
+begin a := (select * from onecol for update); raise notice 'a = %', a; end$$;
-- error cases:
do $$ declare a int[];
-begin a := from onecol; raise notice 'a = %', a; end$$;
+begin a := (select from onecol); raise notice 'a = %', a; end$$;
do $$ declare a int[];
-begin a := f1, f1 from onecol; raise notice 'a = %', a; end$$;
+begin a := (select f1, f1 from onecol); raise notice 'a = %', a; end$$;
insert into onecol values(array[11]);
do $$ declare a int[];
-begin a := f1 from onecol; raise notice 'a = %', a; end$$;
+begin a := (select f1 from onecol); raise notice 'a = %', a; end$$;
do $$ declare a int[];
-begin a := f1 from onecol limit 1; raise notice 'a = %', a; end$$;
+begin a := (select f1 from onecol limit 1); raise notice 'a = %', a; end$$;
do $$ declare a real;
begin a[1] := 2; raise notice 'a = %', a; end$$;
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl b/src/test/recovery/t/026_overwrite_contrecord.pl
index f3e27c19af..80124c084f 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -33,7 +33,7 @@ $node->safe_psql(
'postgres', q{
DO $$
DECLARE
- wal_segsize int := setting::int FROM pg_settings WHERE name = 'wal_segment_size';
+ wal_segsize int := (select setting::int FROM pg_settings WHERE name = 'wal_segment_size');
remain int;
iters int := 0;
BEGIN
@@ -43,7 +43,7 @@ BEGIN
from generate_series(1, 10) g;
remain := wal_segsize - (pg_current_wal_insert_lsn() - '0/0') % wal_segsize;
- IF remain < 2 * setting::int from pg_settings where name = 'block_size' THEN
+ IF (select remain < 2 * setting::int from pg_settings where name = 'block_size') THEN
RAISE log 'exiting after % iterations, % bytes to end of WAL segment', iters, remain;
EXIT;
END IF;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 673361e840..79893df484 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2627,7 +2627,7 @@ LANGUAGE plpgsql AS $$
DECLARE
v_relfilenode oid;
BEGIN
- v_relfilenode := relfilenode FROM pg_class WHERE oid = p_tablename;
+ v_relfilenode := (select relfilenode FROM pg_class WHERE oid = p_tablename);
EXECUTE p_ddl;
diff --git a/src/test/regress/expected/plancache.out b/src/test/regress/expected/plancache.out
index 4e59188196..faae99515f 100644
--- a/src/test/regress/expected/plancache.out
+++ b/src/test/regress/expected/plancache.out
@@ -139,7 +139,7 @@ create temp view v1 as
select 2+2 as f1;
create function cache_test_2() returns int as $$
begin
- return f1 from v1;
+ return (select f1 from v1);
end$$ language plpgsql;
select cache_test_2();
cache_test_2
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 074af8f33a..f2eaac9d69 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -121,7 +121,7 @@ create trigger tg_room_ad after delete
-- ************************************************************
create function tg_wslot_biu() returns trigger as $$
begin
- if count(*) = 0 from Room where roomno = new.roomno then
+ if (select count(*) = 0 from Room where roomno = new.roomno) then
raise exception 'Room % does not exist', new.roomno;
end if;
return new;
@@ -286,7 +286,7 @@ begin
raise exception ''no manual manipulation of HSlot'';
end if;
if tg_op = ''UPDATE'' and new.hubname != old.hubname then
- if count(*) > 0 from Hub where name = old.hubname then
+ if (select count(*) > 0 from Hub where name = old.hubname) then
raise exception ''no manual manipulation of HSlot'';
end if;
end if;
@@ -942,12 +942,12 @@ begin
return retval || pslot_backlink_view(psrec.slotlink);
end if;
if sltype = ''HS'' then
- retval := comment from Hub H, HSlot HS
+ retval := (select comment from Hub H, HSlot HS
where HS.slotname = psrec.slotlink
- and H.name = HS.hubname;
+ and H.name = HS.hubname);
retval := retval || '' slot '';
- retval := retval || slotno::text from HSlot
- where slotname = psrec.slotlink;
+ retval := (select retval || slotno::text from HSlot
+ where slotname = psrec.slotlink);
return retval;
end if;
return psrec.slotlink;
@@ -2388,7 +2388,7 @@ begin
end $$ language plpgsql;
select namedparmcursor_test7();
ERROR: division by zero
-CONTEXT: SQL expression "42/0 AS p1, 77 AS p2"
+CONTEXT: PL/pgSQL stetement OPEN "42/0 AS p1, 77 AS p2"
PL/pgSQL function namedparmcursor_test7() line 6 at OPEN
-- check that line comments work correctly within the argument list
-- (this used to require a special hack in the code; it no longer does,
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 8c4da95508..6335487546 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -327,7 +327,7 @@ CREATE STATISTICS tststats.s8 ON a, b FROM tststats.pt;
CREATE STATISTICS tststats.s9 ON a, b FROM tststats.pt1;
DO $$
DECLARE
- relname text := reltoastrelid::regclass FROM pg_class WHERE oid = 'tststats.t'::regclass;
+ relname text := (select reltoastrelid::regclass FROM pg_class WHERE oid = 'tststats.t'::regclass);
BEGIN
EXECUTE 'CREATE STATISTICS tststats.s10 ON a, b FROM ' || relname;
EXCEPTION WHEN wrong_object_type THEN
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 7f5757e89c..3458dad174 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -543,7 +543,7 @@ select * from xacttest;
rollback;
-- Now the same test with plpgsql (since it depends on SPI which is different)
create or replace function max_xacttest() returns smallint language plpgsql as
-'begin return max(a) from xacttest; end' stable;
+'begin return (select max(a) from xacttest); end' stable;
begin;
update xacttest set a = max_xacttest() + 10 where a > 0;
select * from xacttest;
@@ -558,7 +558,7 @@ select * from xacttest;
rollback;
create or replace function max_xacttest() returns smallint language plpgsql as
-'begin return max(a) from xacttest; end' volatile;
+'begin return (select max(a) from xacttest); end' volatile;
begin;
update xacttest set a = max_xacttest() + 10 where a > 0;
select * from xacttest;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 8c8fa27a6a..62a6c49d81 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1667,7 +1667,7 @@ LANGUAGE plpgsql AS $$
DECLARE
v_relfilenode oid;
BEGIN
- v_relfilenode := relfilenode FROM pg_class WHERE oid = p_tablename;
+ v_relfilenode := (select relfilenode FROM pg_class WHERE oid = p_tablename);
EXECUTE p_ddl;
diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index 4b2f11dcc6..a3dbd93468 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -81,7 +81,7 @@ create temp view v1 as
create function cache_test_2() returns int as $$
begin
- return f1 from v1;
+ return (select f1 from v1);
end$$ language plpgsql;
select cache_test_2();
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 18c91572ae..fbc21b1f52 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -161,7 +161,7 @@ create trigger tg_room_ad after delete
-- ************************************************************
create function tg_wslot_biu() returns trigger as $$
begin
- if count(*) = 0 from Room where roomno = new.roomno then
+ if (select count(*) = 0 from Room where roomno = new.roomno) then
raise exception 'Room % does not exist', new.roomno;
end if;
return new;
@@ -348,7 +348,7 @@ begin
raise exception ''no manual manipulation of HSlot'';
end if;
if tg_op = ''UPDATE'' and new.hubname != old.hubname then
- if count(*) > 0 from Hub where name = old.hubname then
+ if (select count(*) > 0 from Hub where name = old.hubname) then
raise exception ''no manual manipulation of HSlot'';
end if;
end if;
@@ -1071,12 +1071,12 @@ begin
return retval || pslot_backlink_view(psrec.slotlink);
end if;
if sltype = ''HS'' then
- retval := comment from Hub H, HSlot HS
+ retval := (select comment from Hub H, HSlot HS
where HS.slotname = psrec.slotlink
- and H.name = HS.hubname;
+ and H.name = HS.hubname);
retval := retval || '' slot '';
- retval := retval || slotno::text from HSlot
- where slotname = psrec.slotlink;
+ retval := (select retval || slotno::text from HSlot
+ where slotname = psrec.slotlink);
return retval;
end if;
return psrec.slotlink;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index 0c08a6cc42..36cfa31c9f 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -209,7 +209,7 @@ CREATE STATISTICS tststats.s8 ON a, b FROM tststats.pt;
CREATE STATISTICS tststats.s9 ON a, b FROM tststats.pt1;
DO $$
DECLARE
- relname text := reltoastrelid::regclass FROM pg_class WHERE oid = 'tststats.t'::regclass;
+ relname text := (select reltoastrelid::regclass FROM pg_class WHERE oid = 'tststats.t'::regclass);
BEGIN
EXECUTE 'CREATE STATISTICS tststats.s10 ON a, b FROM ' || relname;
EXCEPTION WHEN wrong_object_type THEN
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index 51ae1b31b3..1981955ac6 100644
--- a/src/test/regress/sql/transactions.sql
+++ b/src/test/regress/sql/transactions.sql
@@ -320,7 +320,7 @@ rollback;
-- Now the same test with plpgsql (since it depends on SPI which is different)
create or replace function max_xacttest() returns smallint language plpgsql as
-'begin return max(a) from xacttest; end' stable;
+'begin return (select max(a) from xacttest); end' stable;
begin;
update xacttest set a = max_xacttest() + 10 where a > 0;
@@ -328,7 +328,7 @@ select * from xacttest;
rollback;
create or replace function max_xacttest() returns smallint language plpgsql as
-'begin return max(a) from xacttest; end' volatile;
+'begin return (select max(a) from xacttest); end' volatile;
begin;
update xacttest set a = max_xacttest() + 10 where a > 0;
* Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 22:18 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Tom Lane <tgl@sss.pgh.pa.us>
2024-06-03 16:45 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Erik Wienhold <ewie@ewie.name>
2024-06-03 18:11 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
2024-06-04 06:39 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-10 11:56 ` Re: Missing semicolumn in anonymous plpgsql block does not raise syntax error Pavel Stehule <pavel.stehule@gmail.com>
@ 2024-06-10 15:00 ` Pavel Stehule <pavel.stehule@gmail.com>
0 siblings, 0 replies; 13+ messages in thread
From: Pavel Stehule @ 2024-06-10 15:00 UTC (permalink / raw)
To: Mor Lehr <mor.lehr@deel.com>; +Cc: Erik Wienhold <ewie@ewie.name>; Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
po 10. 6. 2024 v 13:56 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:
> Hi
>
> út 4. 6. 2024 v 8:39 odesílatel Mor Lehr <mor.lehr@deel.com> napsal:
>
>> How about inventing an opt-in strict mode
>>
>>
>> That can be useful at the session level, because we use anonymous blocks
>> quite often.
>> I assume if such a setting existed - we would have used it in the
>> original scenario.
>>
>> Thanks again,
>> -Mor
>>
>> On Mon, Jun 3, 2024 at 9:12 PM Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>>
>>>
>>>
>>> po 3. 6. 2024 v 18:46 odesílatel Erik Wienhold <ewie@ewie.name> napsal:
>>>
>>>> On 2024-06-03 00:18 +0200, Tom Lane wrote:
>>>> > "David G. Johnston" <david.g.johnston@gmail.com> writes:
>>>> > > I think you just wrote the equivalent of:
>>>> > > l_cnt := (select 1 as delete from foo3 where id=1);
>>>> > > Which is a valid query.
>>>> >
>>>> > Still another example of the folly of letting AS be optional.
>>>> > I don't suppose we can ever undo that though.
>>>>
>>>> How about inventing an opt-in strict mode (like in Perl or JavaScript)
>>>> that prevents certain footguns? For example, disallowing bare column
>>>> labels.
>>>>
>>>> That could be enabled for the current session or transaction:
>>>>
>>>> SET strict_parsing = { on | off };
>>>>
>>>> Or just for individual routines:
>>>>
>>>> CREATE PROCEDURE myproc()
>>>> SET strict_parsing = { on | off }
>>>> LANGUAGE plpgsql ...
>>>>
>>>>
>>> Probably it is not bad idea - it can be generally useful
>>>
>>> But I think it is better to introduce a new entry for plpgsql
>>> expressions in gram.y.
>>>
>>> Unfortunately it is not a compatible change. Years ago was popular to
>>> use a pattern
>>>
>>> a := tab.a FROM tab
>>>
>>> instead correct
>>>
>>> a := (SELECT tab.a FROM tab)
>>>
>>> or
>>>
>>> SELECT tab.a FROM tab INTO a;
>>>
>>> Regards
>>>
>>> Pavel
>>>
>>
> I wrote an experimental patch that enforces strict syntax for PL/pgSQL
> expressions. This patch is a proof concept and shows impacts of the change
> (check-world tests passed)
>
> Unfortunately it introduces break compatibility - with this patch the odd
> syntax (that is undocumented) is not supported. Something like
>
> DECLARE _x int := x FROM foo WHERE id = _arg;
>
> should not be written in strict mode;
>
> This patch very well fix reported issue:
>
> (2024-06-10 12:06:43) postgres=# CREATE TABLE foo3(id serial PRIMARY key,
> txt text);
> CREATE TABLE
> (2024-06-10 12:07:13) postgres=# INSERT INTO foo3 (txt) VALUES
> ('aaa'),('bbb');
> INSERT 0 2
> (2024-06-10 12:07:33) postgres=# DO $$
> DECLARE
> l_cnt int;
> BEGIN
> l_cnt := 1
> DELETE FROM foo3 WHERE id=1;
> END; $$;
> ERROR: syntax error at or near "DELETE"
> LINE 11: DELETE FROM foo3 WHERE id=1;
> ^
>
> Personally, I think it can be a strong step forward (we can use deeper
> integration of plpgsql with SQL parser which was not possible before).
>
> Because it introduces a compatibility break I don't propose to use it by
> default. I see two possible ways, how this check can be used:
>
> 1. we can use plpgsql.extra_errors (
> https://www.postgresql.org/docs/current/plpgsql-development-tips.html#PLPGSQL-EXTRA-CHECKS
> ) and introduce a check named (maybe) strict_expr_syntax. Because in this
> case the warning cannot be raised, then this check cannot be used in
> plpgsql.extra_warnings. I think it can work nicely.
>
> 2. if @1 will not be possible, the minimalist implementation can be based
> on a new public entry to SQL parser. In this case, I can do the proposed
> check at least from plpgsql_check.
>
> Do you see any other possibilities?
>
> This patch is just a proof concept. I didn't implement any mechanism to
> switch from default mode to strict mode (I don't propose strict mode as
> default) now.
>
> I think it can increase the robustness of plpgsql, on the other hand it
> introduces compatibility breaks and I understand related problems.
>
> The change of definition of PLpgSQL_Expr has an impact on OPEN and CASE
> PLpgSQL statements that use multicolumn results.
>
Note - the SQL/PSM standard allow syntax
SET var = (SELECT col FROM tab)
as shorter variant of
SELECT col INTO var FROM tab ;
so
var := (SELECT col FROM tab)
is +/- ANSI/SQL syntax. It is not my invention. The subquery is used as a
guard against returning multiple rows.
The proprietary Postgre syntax is a little bit faster - 80us x 95 us,
doesn't raise an exception when returning more rows (I am not sure if this
is any benefit or not - it is possibly dangerous, but it can reduce the
necessity of subtransaction in some patterns). Instead of proprietary
syntax, SELECT INTO can be used for these cases.
Regards
Pavel
> Regards
>
> Pavel
>
>
>>
>>>
>>>
>>>> --
>>>> Erik
>>>>
>>>>
>>>>
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2025-02-06 10:10 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-06-02 11:24 Missing semicolumn in anonymous plpgsql block does not raise syntax error Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:19 ` David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:26 ` Mor Lehr <mor.lehr@deel.com>
2024-06-02 15:31 ` David G. Johnston <david.g.johnston@gmail.com>
2024-06-02 15:42 ` Mor Lehr <mor.lehr@deel.com>
2025-02-06 06:47 ` Pavel Stehule <pavel.stehule@gmail.com>
2025-02-06 10:10 ` Mor Lehr <mor.lehr@deel.com>
2024-06-02 22:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-06-03 16:45 ` Erik Wienhold <ewie@ewie.name>
2024-06-03 18:11 ` Pavel Stehule <pavel.stehule@gmail.com>
2024-06-04 06:39 ` Mor Lehr <mor.lehr@deel.com>
2024-06-10 11:56 ` Pavel Stehule <pavel.stehule@gmail.com>
2024-06-10 15:00 ` Pavel Stehule <pavel.stehule@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox