Missing semicolumn in anonymous plpgsql block does not raise syntax error

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.

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:

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.

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:

"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

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

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

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:

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

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

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

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: