proposal: plpgsql, new check for extra_errors - strict_expr_check

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.stehule@gmail.com>
escreveu:

Hi

ne 16. 6. 2024 v 16:22 odesílatel Marcos Pegoraro <marcos@f10.com.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.

Em dom., 16 de jun. de 2024 às 11:37, Pavel Stehule <pavel.stehule@gmail.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@f10.com.br>
napsal:

Em dom., 16 de jun. de 2024 às 11:37, Pavel Stehule <
pavel.stehule@gmail.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

Em dom., 16 de jun. de 2024 às 12:11, Pavel Stehule <pavel.stehule@gmail.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@f10.com.br>
napsal:

Em dom., 16 de jun. de 2024 às 12:11, Pavel Stehule <
pavel.stehule@gmail.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

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

Hi,

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

/messages/by-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

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@darold.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

Hi

only rebase

Regards

Pavel

Hi

fresh rebase

Regards

Pavel

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed

This is properly isolated, only available when turned on.
I ran this through the examples as specified. Then through OUR 1,000 + procedures.
I used \set ON_ERROR_STOP 'on'
and loaded a script that recreates all of our procedures/functions.
In both modes. And it found nothing. (As expected, but that is 1,000 procedures from the wild).
I forced 1 procedure to have the problem.
As expected, when enabled, this STOPPED and pointed out the error.
Without this setting, the code loaded clean.

Small note: The doc says this check is allowed only when plpgsql.extra_errors is set to "strict_expr_check"...
But it also is obviously in effect when that is set to 'all' for reasons that should be completely obvious.

The new status of this patch is: Ready for Committer

Hi

st 30. 7. 2025 v 0:17 odesílatel Kirk Wolak <wolakk@gmail.com> napsal:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed

This is properly isolated, only available when turned on.
I ran this through the examples as specified. Then through OUR 1,000 +
procedures.
I used \set ON_ERROR_STOP 'on'
and loaded a script that recreates all of our procedures/functions.
In both modes. And it found nothing. (As expected, but that is 1,000
procedures from the wild).
I forced 1 procedure to have the problem.
As expected, when enabled, this STOPPED and pointed out the error.
Without this setting, the code loaded clean.

Small note: The doc says this check is allowed only when
plpgsql.extra_errors is set to "strict_expr_check"...
But it also is obviously in effect when that is set to 'all' for reasons
that should be completely obvious.

I removed this sentence from doc. Now, it is consistent with other plpgsql
extra checks.

The new status of this patch is: Ready for Committer

thank you for review

updated patch attached (only documentation was changed, one sentence
removed)

Regards

Pavel

Hi

only rebase

Regards

Pavel