proposal: plpgsql, new check for extra_errors - strict_expr_check

Hi,

assigned patch try to solve issue reported by Mor Lehr (Missing semicolon
in anonymous plpgsql block does not raise syntax error).

https://www.postgresql.org/message-id/CALyvM2bp_CXMH_Gyq87pmHJRuZDEhV40u9VP8rX=CAnEt2wUXg@mail.gmail.com

by introducing a new extra error check. With this check only a_expr exprs
are allowed as plpgsql expressions. This is a small step to behaviour
described in SQL/PSM standard (although the language is different, the
expression syntax and features are almost similar. With this check the
undocumented (but supported syntax)

var := column FROM tab

is disallowed. Only ANSI syntax for embedded queries (inside assignment
statement) is allowed

var := (SELECT column FROM tab);

With this check, the reported issue (by Mor Lehr) is detected

default setting

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; $$

-- without reaction - just don't work

(2024-06-16 16:05:55) postgres=# set plpgsql.extra_errors to
'strict_expr_check';
SET
(2024-06-16 16:06:43) 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;
^

This patch has three parts

1. Introduction strict_expr_check
2. set strict_expr_check as default, and impact on regress tests
3. revert @2

I don't propose to be strict_expr_check active by default.

Comments, notes?

Regards

Pavel

Can you remove or just ignore double ; too ?

postgres=# do $$
declare var_x integer;
begin
var_x = 99;;
delete from x where x = var_x;
end; $$;
ERROR: syntax error at or near ";"
LINE 1: do $$ declare var_x integer; begin var_x = 99;; delete from ...

Atenciosamente,

Em dom., 16 de jun. de 2024 às 11:12, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
escreveu:

> Hi,
>
> assigned patch try to solve issue reported by Mor Lehr (Missing semicolon
> in anonymous plpgsql block does not raise syntax error).
>
>
> https://www.postgresql.org/message-id/CALyvM2bp_CXMH_Gyq87pmHJRuZDEhV40u9VP8rX=CAnEt2wUXg@mail.gmail.com
>
> by introducing a new extra error check. With this check only a_expr exprs
> are allowed as plpgsql expressions. This is a small step to behaviour
> described in SQL/PSM standard (although the language is different, the
> expression syntax and features are almost similar. With this check the
> undocumented (but supported syntax)
>
> var := column FROM tab
>
> is disallowed. Only ANSI syntax for embedded queries (inside assignment
> statement) is allowed
>
> var := (SELECT column FROM tab);
>
> With this check, the reported issue (by Mor Lehr) is detected
>
> default setting
>
> 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; $$
>
> -- without reaction - just don't work
>
> (2024-06-16 16:05:55) postgres=# set plpgsql.extra_errors to
> 'strict_expr_check';
> SET
> (2024-06-16 16:06:43) 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;
> ^
>
> This patch has three parts
>
> 1. Introduction strict_expr_check
> 2. set strict_expr_check as default, and impact on regress tests
> 3. revert @2
>
> I don't propose to be strict_expr_check active by default.
>
> Comments, notes?
>
> Regards
>
> Pavel
>
>
>

Hi

ne 16. 6. 2024 v 16:22 odesílatel Marcos Pegoraro <marcos(at)f10(dot)com(dot)br>
napsal:

> Can you remove or just ignore double ; too ?
>

I don't know - it is a different issue.

PLpgSQL allows zero statements inside block, so you can write BEGIN END or
IF 1 THEN END IF but it doesn't allow empty statement

like ;;

probably it just needs one more rule in gram.y - but in this case, I am not
sure if we should support it.

What is the expected benefit? Generally PL/pgSQL has very strict syntax -
and using double semicolons makes no sense.

>
> postgres=# do $$
> declare var_x integer;
> begin
> var_x = 99;;
> delete from x where x = var_x;
> end; $$;
> ERROR: syntax error at or near ";"
> LINE 1: do $$ declare var_x integer; begin var_x = 99;; delete from ...
>
> Atenciosamente,
>
>
>
>
> Em dom., 16 de jun. de 2024 às 11:12, Pavel Stehule <
> pavel(dot)stehule(at)gmail(dot)com> escreveu:
>
>> Hi,
>>
>> assigned patch try to solve issue reported by Mor Lehr (Missing semicolon
>> in anonymous plpgsql block does not raise syntax error).
>>
>>
>> https://www.postgresql.org/message-id/CALyvM2bp_CXMH_Gyq87pmHJRuZDEhV40u9VP8rX=CAnEt2wUXg@mail.gmail.com
>>
>> by introducing a new extra error check. With this check only a_expr exprs
>> are allowed as plpgsql expressions. This is a small step to behaviour
>> described in SQL/PSM standard (although the language is different, the
>> expression syntax and features are almost similar. With this check the
>> undocumented (but supported syntax)
>>
>> var := column FROM tab
>>
>> is disallowed. Only ANSI syntax for embedded queries (inside assignment
>> statement) is allowed
>>
>> var := (SELECT column FROM tab);
>>
>> With this check, the reported issue (by Mor Lehr) is detected
>>
>> default setting
>>
>> 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; $$
>>
>> -- without reaction - just don't work
>>
>> (2024-06-16 16:05:55) postgres=# set plpgsql.extra_errors to
>> 'strict_expr_check';
>> SET
>> (2024-06-16 16:06:43) 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;
>> ^
>>
>> This patch has three parts
>>
>> 1. Introduction strict_expr_check
>> 2. set strict_expr_check as default, and impact on regress tests
>> 3. revert @2
>>
>> I don't propose to be strict_expr_check active by default.
>>
>> Comments, notes?
>>
>> Regards
>>
>> Pavel
>>
>>
>>

Em dom., 16 de jun. de 2024 às 11:37, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
escreveu:

>
> What is the expected benefit? Generally PL/pgSQL has very strict syntax -
> and using double semicolons makes no sense.
>
> exactly, makes no sense. That is because it should be ignored, right ?
But ok, if this is a different issue, that´s fine.

regards
Marcos

ne 16. 6. 2024 v 16:43 odesílatel Marcos Pegoraro <marcos(at)f10(dot)com(dot)br>
napsal:

> Em dom., 16 de jun. de 2024 às 11:37, Pavel Stehule <
> pavel(dot)stehule(at)gmail(dot)com> escreveu:
>
>>
>> What is the expected benefit? Generally PL/pgSQL has very strict syntax -
>> and using double semicolons makes no sense.
>>
>> exactly, makes no sense. That is because it should be ignored, right ?
> But ok, if this is a different issue, that´s fine.
>

I don't follow this idea - when it does not make sense, then why do you use
it? It can be a signal of some issue in your code.

The source code should not contain a code that should be ignored.

But I am not a authority - can be interesting if this is allowed in PL/SQL
or Ada

Regards

Pavel

> regards
> Marcos
>

Em dom., 16 de jun. de 2024 às 12:11, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
escreveu:

> I don't follow this idea - when it does not make sense, then why do you
> use it? It can be a signal of some issue in your code.
>
>>
I don't use it, but sometimes it occurs, and there are lots of languages
which ignore it, so it would be cool if plpgsql does it too.

If you do this, works
set search_path to public;;;

but if you do the same inside a block, it does not.

regards
Marcos

ne 16. 6. 2024 v 19:36 odesílatel Marcos Pegoraro <marcos(at)f10(dot)com(dot)br>
napsal:

> Em dom., 16 de jun. de 2024 às 12:11, Pavel Stehule <
> pavel(dot)stehule(at)gmail(dot)com> escreveu:
>
>> I don't follow this idea - when it does not make sense, then why do you
>> use it? It can be a signal of some issue in your code.
>>
>>>
> I don't use it, but sometimes it occurs, and there are lots of languages
> which ignore it, so it would be cool if plpgsql does it too.
>
> If you do this, works
> set search_path to public;;;
>

psql allows it, but it is a shell - not a programming language.

>
> but if you do the same inside a block, it does not.
>

It is a different language. I have not too strong an opinion about it - it
is hard to say what is the correct design when you should work with a mix
of languages like SQL and Ada (PL/pgSQL), and when related standard SQL/PSM
is not widely used. Personally, I don't see any nice features that allow it
to accept dirty code. I have negative experiences when a language is
tolerant.

Regards

Pavel

> regards
> Marcos
>

ne 16. 6. 2024 v 16:11 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

> Hi,
>
> assigned patch try to solve issue reported by Mor Lehr (Missing semicolon
> in anonymous plpgsql block does not raise syntax error).
>
>
> https://www.postgresql.org/message-id/CALyvM2bp_CXMH_Gyq87pmHJRuZDEhV40u9VP8rX=CAnEt2wUXg@mail.gmail.com
>
> by introducing a new extra error check. With this check only a_expr exprs
> are allowed as plpgsql expressions. This is a small step to behaviour
> described in SQL/PSM standard (although the language is different, the
> expression syntax and features are almost similar. With this check the
> undocumented (but supported syntax)
>
> var := column FROM tab
>
> is disallowed. Only ANSI syntax for embedded queries (inside assignment
> statement) is allowed
>
> var := (SELECT column FROM tab);
>
> With this check, the reported issue (by Mor Lehr) is detected
>
> default setting
>
> 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; $$
>
> -- without reaction - just don't work
>
> (2024-06-16 16:05:55) postgres=# set plpgsql.extra_errors to
> 'strict_expr_check';
> SET
> (2024-06-16 16:06:43) 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;
> ^
>
> This patch has three parts
>
> 1. Introduction strict_expr_check
> 2. set strict_expr_check as default, and impact on regress tests
> 3. revert @2
>
> I don't propose to be strict_expr_check active by default.
>
> Comments, notes?
>

fresh rebase

>
> Regards
>
> Pavel
>
>
>

Hi

fresh rebase

Regards

Pavel

Hi

fresh rebase

Regards

Pavel

Hi

I rewrote this patch. Instead of enhancing the main SQL parser, it does
post parser checks of the parse tree.

Now the patch is significantly less invasive (changes are just in plpgsql -
mostly in grammar), and it is smaller (without regress tests it has half
size).

This patch allows the detection of usage of undocumented syntax for plpgsql
expressions. Using this undocumented
syntax can be the reason why badly written code (missing semicolon) can be
quietly executed without any raising of error.

Only patch 01 is important - patches 02, 03 are prepared for review.
Patch 02 activates a new check by default, and fixes the regress test to be
executed. This is important for checking for possible false alarms.
Patch 03 disables this check and returns regress tests to their original
state.

Regards

Pavel

Le 07/02/2025 à 23:00, Pavel Stehule a écrit :
> Hi
>
> I rewrote this patch. Instead of enhancing the main SQL parser, it
> does post parser checks of the parse tree.
>
> Now the patch is significantly less invasive (changes are just in
> plpgsql - mostly in grammar), and it is smaller (without regress tests
> it has half size).
>
> This patch allows the detection of usage of undocumented syntax for
> plpgsql expressions. Using this undocumented
> syntax can be the reason why badly written code (missing semicolon)
> can be quietly executed without any raising of error.
>
> Only patch 01 is important - patches 02, 03 are prepared for review.
> Patch 02 activates a new check by default, and fixes the regress test
> to be executed. This is important for checking for possible false alarms.
> Patch 03 disables this check and returns regress tests to their
> original state.
>
> Regards
>
> Pavel

Hi Pavel,

I'm reviewing this patch too and I'm facing some documentation  issues
in patch
v20250207-0001-use-strict-rules-for-parsing-PL-pgSQL-expressions.patch

+        it doesn't to allow to detect broken code.

I'm not very good at english but I think it should be: it doesn't allow
to detect broken code.

Here I think the sentence is not complete:
+        This check is allowed only <varname>plpgsql.extra_errors</varname>.

Do you mean: This check is allowed only when
<varname>plpgsql.extra_errors</varname> is set to 'strict_expr_check'.

Please fix these to be sure of what the code is supposed to do.

Thanks

--
Gilles Darold
http://www.darold.net/

Hi

čt 27. 2. 2025 v 16:33 odesílatel Gilles Darold <gilles(at)darold(dot)net> napsal:

> Le 07/02/2025 à 23:00, Pavel Stehule a écrit :
> > Hi
> >
> > I rewrote this patch. Instead of enhancing the main SQL parser, it
> > does post parser checks of the parse tree.
> >
> > Now the patch is significantly less invasive (changes are just in
> > plpgsql - mostly in grammar), and it is smaller (without regress tests
> > it has half size).
> >
> > This patch allows the detection of usage of undocumented syntax for
> > plpgsql expressions. Using this undocumented
> > syntax can be the reason why badly written code (missing semicolon)
> > can be quietly executed without any raising of error.
> >
> > Only patch 01 is important - patches 02, 03 are prepared for review.
> > Patch 02 activates a new check by default, and fixes the regress test
> > to be executed. This is important for checking for possible false alarms.
> > Patch 03 disables this check and returns regress tests to their
> > original state.
> >
> > Regards
> >
> > Pavel
>
> Hi Pavel,
>
> I'm reviewing this patch too and I'm facing some documentation issues
> in patch
> v20250207-0001-use-strict-rules-for-parsing-PL-pgSQL-expressions.patch
>
> + it doesn't to allow to detect broken code.
>
> I'm not very good at english but I think it should be: it doesn't allow
> to detect broken code.
>

fixed

>
> Here I think the sentence is not complete:
> + This check is allowed only
> <varname>plpgsql.extra_errors</varname>.
>
> Do you mean: This check is allowed only when
> <varname>plpgsql.extra_errors</varname> is set to 'strict_expr_check'.
>
> Please fix these to be sure of what the code is supposed to do.
>

fixed

Regards

Pavel

>
>
> Thanks
>
> --
> Gilles Darold
> http://www.darold.net/
>
>