ne 16. 6. 2024 v 16:11 odesÃlatel Pavel Stehule <pavel.steh...@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). > > > https://www.postgresql.org/message-id/CALyvM2bp_CXMH_Gyq87pmHJRuZDEhV40u9VP8rX=canet2w...@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 > > >
From ebe7c73534f160d75356ac605cdb6a147e806f8f Mon Sep 17 00:00:00 2001 From: "ok...@github.com" <pavel.steh...@gmail.com> Date: Sun, 16 Jun 2024 08:13:53 +0200 Subject: [PATCH 2/3] simply check of strict-expr-check on regress test This patch enable strict-expr-check by default to be possible to see the impact of this option on regress test. Next commit will revert this option. The strict-expr-check should not be enabled by default. This commit is done just for testing. --- .../basic_archive/expected/basic_archive.out | 4 +-- contrib/basic_archive/sql/basic_archive.sql | 4 +-- src/pl/plpgsql/src/expected/plpgsql_array.out | 34 +++++++++++-------- src/pl/plpgsql/src/pl_handler.c | 4 +-- src/pl/plpgsql/src/sql/plpgsql_array.sql | 14 ++++---- .../recovery/t/026_overwrite_contrecord.pl | 4 +-- src/test/regress/expected/alter_table.out | 2 +- src/test/regress/expected/plancache.out | 2 +- src/test/regress/expected/plpgsql.out | 12 +++---- src/test/regress/expected/stats_ext.out | 2 +- src/test/regress/expected/transactions.out | 4 +-- src/test/regress/sql/alter_table.sql | 2 +- src/test/regress/sql/plancache.sql | 2 +- src/test/regress/sql/plpgsql.sql | 12 +++---- src/test/regress/sql/stats_ext.sql | 2 +- src/test/regress/sql/transactions.sql | 4 +-- 16 files changed, 56 insertions(+), 52 deletions(-) diff --git a/contrib/basic_archive/expected/basic_archive.out b/contrib/basic_archive/expected/basic_archive.out index 0015053e0f2..280ff3e022e 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 14e236d57ab..2c127a821f1 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/pl/plpgsql/src/expected/plpgsql_array.out b/src/pl/plpgsql/src/expected/plpgsql_array.out index ad60e0e8be3..6819c0435f6 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_handler.c b/src/pl/plpgsql/src/pl_handler.c index e7037fa8e81..fa6e2ab1d1f 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -50,7 +50,7 @@ bool plpgsql_check_asserts = true; static char *plpgsql_extra_warnings_string = NULL; static char *plpgsql_extra_errors_string = NULL; int plpgsql_extra_warnings; -int plpgsql_extra_errors; +int plpgsql_extra_errors = PLPGSQL_XCHECK_STRICTEXPRCHECK; /* Hook for plugins */ PLpgSQL_plugin **plpgsql_plugin_ptr = NULL; @@ -193,7 +193,7 @@ _PG_init(void) gettext_noop("List of programming constructs that should produce an error."), NULL, &plpgsql_extra_errors_string, - "none", + "strict_expr_check", PGC_USERSET, GUC_LIST_INPUT, plpgsql_extra_checks_check_hook, plpgsql_extra_errors_assign_hook, diff --git a/src/pl/plpgsql/src/sql/plpgsql_array.sql b/src/pl/plpgsql/src/sql/plpgsql_array.sql index 4b9ff515948..699713696d4 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 f3e27c19af5..da1add175ac 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 79cf82b5aed..fd9e10e909c 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 4e59188196c..faae99515f7 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 0a6945581bd..0e25138b918 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; diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out index 8c4da955084..e2899a0e888 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 7f5757e89c4..3458dad1749 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 28cabc49e9f..845e23d8ae2 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 4b2f11dcc64..a3dbd93468e 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 18c91572ae1..fbc21b1f52e 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 0c08a6cc42e..f2977c8ad38 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 51ae1b31b30..1981955ac65 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; -- 2.46.0
From 86b6dcb7a7e5dec3e5796270fa83a2465e803fed Mon Sep 17 00:00:00 2001 From: "ok...@github.com" <pavel.steh...@gmail.com> Date: Sun, 16 Jun 2024 15:52:38 +0200 Subject: [PATCH 3/3] set plpgsql.extra_errors to "none" Purpose of previous commit was to run tests with active strict_expr_check. Now, reset to default and revert all changes from previous commit. --- .../basic_archive/expected/basic_archive.out | 4 +-- contrib/basic_archive/sql/basic_archive.sql | 4 +-- src/pl/plpgsql/src/expected/plpgsql_array.out | 34 ++++++++----------- src/pl/plpgsql/src/pl_handler.c | 4 +-- src/pl/plpgsql/src/sql/plpgsql_array.sql | 14 ++++---- .../recovery/t/026_overwrite_contrecord.pl | 4 +-- src/test/regress/expected/alter_table.out | 2 +- src/test/regress/expected/plancache.out | 2 +- src/test/regress/expected/plpgsql.out | 12 +++---- src/test/regress/expected/stats_ext.out | 2 +- src/test/regress/expected/transactions.out | 4 +-- src/test/regress/sql/alter_table.sql | 2 +- src/test/regress/sql/plancache.sql | 2 +- src/test/regress/sql/plpgsql.sql | 12 +++---- src/test/regress/sql/stats_ext.sql | 2 +- src/test/regress/sql/transactions.sql | 4 +-- 16 files changed, 52 insertions(+), 56 deletions(-) diff --git a/contrib/basic_archive/expected/basic_archive.out b/contrib/basic_archive/expected/basic_archive.out index 280ff3e022e..0015053e0f2 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 := (SELECT count(*) > 0 FROM pg_ls_dir('.', false, false) a - WHERE a ~ '^[0-9A-F]{24}$'); + archived := 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 2c127a821f1..14e236d57ab 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 := (SELECT count(*) > 0 FROM pg_ls_dir('.', false, false) a - WHERE a ~ '^[0-9A-F]{24}$'); + archived := 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/pl/plpgsql/src/expected/plpgsql_array.out b/src/pl/plpgsql/src/expected/plpgsql_array.out index 6819c0435f6..ad60e0e8be3 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_array.out +++ b/src/pl/plpgsql/src/expected/plpgsql_array.out @@ -50,38 +50,34 @@ 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 := (select array_agg(x) from (values(1),(2),(3)) v(x)); raise notice 'a = %', a; end$$; +begin a := 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 := (select f1 from onecol); raise notice 'a = %', a; end$$; +begin a := f1 from onecol; raise notice 'a = %', a; end$$; NOTICE: a = {1,2} do $$ declare a int[]; -begin a := (select * from onecol for update); raise notice 'a = %', a; end$$; +begin a := * from onecol for update; raise notice 'a = %', a; end$$; NOTICE: a = {1,2} -- error cases: do $$ declare a int[]; -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 +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 do $$ declare a int[]; -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 +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 insert into onecol values(array[11]); do $$ declare a int[]; -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)" +begin a := f1 from onecol; raise notice 'a = %', a; end$$; +ERROR: query returned more than one row +CONTEXT: query: a := f1 from onecol PL/pgSQL function inline_code_block line 2 at assignment do $$ declare a int[]; -begin a := (select f1 from onecol limit 1); raise notice 'a = %', a; end$$; +begin a := 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_handler.c b/src/pl/plpgsql/src/pl_handler.c index fa6e2ab1d1f..e7037fa8e81 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -50,7 +50,7 @@ bool plpgsql_check_asserts = true; static char *plpgsql_extra_warnings_string = NULL; static char *plpgsql_extra_errors_string = NULL; int plpgsql_extra_warnings; -int plpgsql_extra_errors = PLPGSQL_XCHECK_STRICTEXPRCHECK; +int plpgsql_extra_errors; /* Hook for plugins */ PLpgSQL_plugin **plpgsql_plugin_ptr = NULL; @@ -193,7 +193,7 @@ _PG_init(void) gettext_noop("List of programming constructs that should produce an error."), NULL, &plpgsql_extra_errors_string, - "strict_expr_check", + "none", PGC_USERSET, GUC_LIST_INPUT, plpgsql_extra_checks_check_hook, plpgsql_extra_errors_assign_hook, diff --git a/src/pl/plpgsql/src/sql/plpgsql_array.sql b/src/pl/plpgsql/src/sql/plpgsql_array.sql index 699713696d4..4b9ff515948 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 := (select array_agg(x) from (values(1),(2),(3)) v(x)); raise notice 'a = %', a; end$$; +begin a := 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 := (select f1 from onecol); raise notice 'a = %', a; end$$; +begin a := f1 from onecol; raise notice 'a = %', a; end$$; do $$ declare a int[]; -begin a := (select * from onecol for update); raise notice 'a = %', a; end$$; +begin a := * from onecol for update; raise notice 'a = %', a; end$$; -- error cases: do $$ declare a int[]; -begin a := (select from onecol); raise notice 'a = %', a; end$$; +begin a := from onecol; raise notice 'a = %', a; end$$; do $$ declare a int[]; -begin a := (select f1, f1 from onecol); raise notice 'a = %', a; end$$; +begin a := f1, f1 from onecol; raise notice 'a = %', a; end$$; insert into onecol values(array[11]); do $$ declare a int[]; -begin a := (select f1 from onecol); raise notice 'a = %', a; end$$; +begin a := f1 from onecol; raise notice 'a = %', a; end$$; do $$ declare a int[]; -begin a := (select f1 from onecol limit 1); raise notice 'a = %', a; end$$; +begin a := 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 da1add175ac..f3e27c19af5 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 := (SELECT setting::int FROM pg_settings WHERE name = 'wal_segment_size'); + wal_segsize int := 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 (SELECT remain < 2 * setting::int from pg_settings where name = 'block_size') THEN + IF 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 fd9e10e909c..79cf82b5aed 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 := (SELECT relfilenode FROM pg_class WHERE oid = p_tablename); + v_relfilenode := 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 faae99515f7..4e59188196c 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 (select f1 from v1); + return 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 0e25138b918..0a6945581bd 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 (select count(*) = 0 from Room where roomno = new.roomno) then + if 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 (select count(*) > 0 from Hub where name = old.hubname) then + if 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 := (select comment from Hub H, HSlot HS + retval := 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 := (select retval || slotno::text from HSlot - where slotname = psrec.slotlink); + retval := retval || slotno::text from HSlot + where slotname = psrec.slotlink; return retval; end if; return psrec.slotlink; diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out index e2899a0e888..8c4da955084 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 := (SELECT reltoastrelid::regclass FROM pg_class WHERE oid = 'tststats.t'::regclass); + relname text := 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 3458dad1749..7f5757e89c4 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 (select max(a) from xacttest); end' stable; +'begin return 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 (select max(a) from xacttest); end' volatile; +'begin return 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 845e23d8ae2..28cabc49e9f 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 := (SELECT relfilenode FROM pg_class WHERE oid = p_tablename); + v_relfilenode := 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 a3dbd93468e..4b2f11dcc64 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 (select f1 from v1); + return 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 fbc21b1f52e..18c91572ae1 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 (select count(*) = 0 from Room where roomno = new.roomno) then + if 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 (select count(*) > 0 from Hub where name = old.hubname) then + if 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 := (select comment from Hub H, HSlot HS + retval := 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 := (select retval || slotno::text from HSlot - where slotname = psrec.slotlink); + retval := 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 f2977c8ad38..0c08a6cc42e 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 := (SELECT reltoastrelid::regclass FROM pg_class WHERE oid = 'tststats.t'::regclass); + relname text := 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 1981955ac65..51ae1b31b30 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 (select max(a) from xacttest); end' stable; +'begin return 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 (select max(a) from xacttest); end' volatile; +'begin return max(a) from xacttest; end' volatile; begin; update xacttest set a = max_xacttest() + 10 where a > 0; -- 2.46.0
From 6c61739847d5d1af9da1153e1429c81bb8706aab Mon Sep 17 00:00:00 2001 From: "ok...@github.com" <pavel.steh...@gmail.com> Date: Wed, 12 Jun 2024 21:34:05 +0200 Subject: [PATCH 1/3] use strict rules for parsing PL/pgSQL expressions Originally the rule PLpgSQL_Expr allows almost all SQL clauses. It was designed to allow old undocumented syntax var := col FROM tab; The reason for support of this "strange" syntax was technical. The PLpgSQL parser cannot 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 introduce new extra error check strict_expr_check, that solve this issue. --- doc/src/sgml/plpgsql.sgml | 18 ++++ src/backend/executor/spi.c | 6 ++ src/backend/parser/gram.y | 149 +++++++++++++++++++++++++++ src/backend/parser/parser.c | 6 ++ src/include/parser/parser.h | 22 ++++ src/interfaces/ecpg/preproc/parse.pl | 10 +- src/pl/plpgsql/src/pl_comp.c | 31 ++++++ src/pl/plpgsql/src/pl_gram.y | 48 ++++++--- src/pl/plpgsql/src/pl_handler.c | 2 + src/pl/plpgsql/src/plpgsql.h | 19 ++++ 10 files changed, 297 insertions(+), 14 deletions(-) diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index 78e4983139b..a27700d6cb3 100644 --- a/doc/src/sgml/plpgsql.sgml +++ b/doc/src/sgml/plpgsql.sgml @@ -5386,6 +5386,24 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ </para> </listitem> </varlistentry> + + <varlistentry id="plpgsql-extra-checks-strict-expr-check"> + <term><varname>strict_expr_check</varname></term> + <listitem> + <para> + Enabling this check will cause <application>PL/pgSQL</application> to + check if a <application>PL/pgSQL</application> expression is just an + expression without any SQL clauses like <literal>FROM</literal>, + <literal>ORDER BY</literal>. This undocumented form of expressions + is allowed for compatibility reasons, but in some special cases + it doesn't to allow to detect broken code. + </para> + + <para> + This check is allowed only <varname>plpgsql.extra_errors</varname>. + </para> + </listitem> + </varlistentry> </variablelist> The following example shows the effect of <varname>plpgsql.extra_warnings</varname> diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 90d98345764..76c25c9948d 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -2978,11 +2978,17 @@ _SPI_error_callback(void *arg) switch (carg->mode) { case RAW_PARSE_PLPGSQL_EXPR: + case RAW_PARSE_PLPGSQL_STRICT_EXPR: + case RAW_PARSE_PLPGSQL_STRICT_EXPR_LIST: + case RAW_PARSE_PLPGSQL_STRICT_NAMED_EXPR_LIST: errcontext("PL/pgSQL expression \"%s\"", query); break; case RAW_PARSE_PLPGSQL_ASSIGN1: case RAW_PARSE_PLPGSQL_ASSIGN2: case RAW_PARSE_PLPGSQL_ASSIGN3: + case RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN1: + case RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN2: + case RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN3: errcontext("PL/pgSQL assignment \"%s\"", query); break; default: diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 84cef57a707..5abce73b030 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -323,6 +323,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 + PLpgSQLStrictExpr PLpgSQLStrictExprs PLpgSQLStrictNamedExprs + PLAssignStmtStrictExpr + +%type <target> plpgsql_strict_expr plpgsql_strict_named_expr +%type <list> plpgsql_strict_expr_list plpgsql_strict_named_expr_list %type <str> opt_single_name %type <list> opt_qualified_name @@ -824,6 +829,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %token MODE_PLPGSQL_ASSIGN1 %token MODE_PLPGSQL_ASSIGN2 %token MODE_PLPGSQL_ASSIGN3 +%token MODE_PLPGSQL_STRICT_EXPR +%token MODE_PLPGSQL_STRICT_EXPR_LIST +%token MODE_PLPGSQL_STRICT_NAMED_EXPR_LIST +%token MODE_PLPGSQL_STRICT_EXPR_ASSIGN1 +%token MODE_PLPGSQL_STRICT_EXPR_ASSIGN2 +%token MODE_PLPGSQL_STRICT_EXPR_ASSIGN3 /* Precedence: lowest to highest */ @@ -955,6 +966,46 @@ parse_toplevel: pg_yyget_extra(yyscanner)->parsetree = list_make1(makeRawStmt((Node *) n, 0)); } + | MODE_PLPGSQL_STRICT_EXPR PLpgSQLStrictExpr + { + pg_yyget_extra(yyscanner)->parsetree = + list_make1(makeRawStmt($2, 0)); + } + | MODE_PLPGSQL_STRICT_EXPR_LIST PLpgSQLStrictExprs + { + pg_yyget_extra(yyscanner)->parsetree = + list_make1(makeRawStmt($2, 0)); + } + | MODE_PLPGSQL_STRICT_NAMED_EXPR_LIST PLpgSQLStrictNamedExprs + { + pg_yyget_extra(yyscanner)->parsetree = + list_make1(makeRawStmt($2, 0)); + } + | MODE_PLPGSQL_STRICT_EXPR_ASSIGN1 PLAssignStmtStrictExpr + { + PLAssignStmt *n = (PLAssignStmt *) $2; + + n->nnames = 1; + pg_yyget_extra(yyscanner)->parsetree = + list_make1(makeRawStmt((Node *) n, 0)); + } + | MODE_PLPGSQL_STRICT_EXPR_ASSIGN2 PLAssignStmtStrictExpr + { + PLAssignStmt *n = (PLAssignStmt *) $2; + + n->nnames = 2; + pg_yyget_extra(yyscanner)->parsetree = + list_make1(makeRawStmt((Node *) n, 0)); + } + | MODE_PLPGSQL_STRICT_EXPR_ASSIGN3 PLAssignStmtStrictExpr + { + PLAssignStmt *n = (PLAssignStmt *) $2; + + n->nnames = 3; + pg_yyget_extra(yyscanner)->parsetree = + list_make1(makeRawStmt((Node *) n, 0)); + } + ; /* @@ -17426,6 +17477,104 @@ plassign_equals: COLON_EQUALS | '=' ; +/* + * In "strict" mode plpgsql expressions are just an a_expr. From compatibility + * reasons (with default mode) it returns SelectStmt still. + */ +PLpgSQLStrictExpr: a_expr + { + SelectStmt *n = makeNode(SelectStmt); + ResTarget *rt = makeNode(ResTarget); + + rt->name = NULL; + rt->indirection = NIL; + rt->val = (Node *) $1; + rt->location = @1; + + n->targetList = list_make1((Node *) rt); + $$ = (Node *) n; + } + ; + +PLAssignStmtStrictExpr: plassign_target opt_indirection plassign_equals PLpgSQLStrictExpr + { + PLAssignStmt *n = makeNode(PLAssignStmt); + + n->name = $1; + n->indirection = check_indirection($2, yyscanner); + /* nnames will be filled by calling production */ + n->val = (SelectStmt *) $4; + n->location = @1; + $$ = (Node *) n; + } + ; + +/* + * Used for unnamed plpgsql cursor's argument and plpgsql case in + * "strict" mode. + */ +PLpgSQLStrictExprs: plpgsql_strict_expr_list + { + SelectStmt *n = makeNode(SelectStmt); + + n->targetList = $1; + $$ = (Node *) n; + } + ; + +plpgsql_strict_expr_list: + plpgsql_strict_expr + { + $$ = list_make1($1); + } + | plpgsql_strict_expr_list ',' plpgsql_strict_expr + { + $$ = lappend($1, $3); + } + ; + +plpgsql_strict_expr: a_expr + { + $$ = makeNode(ResTarget); + $$->name = NULL; + $$->indirection = NIL; + $$->val = (Node *) $1; + $$->location = @1; + } + ; + +/* + * Used for named cursor's arguments in "strict" mode + */ +PLpgSQLStrictNamedExprs: plpgsql_strict_named_expr_list + { + SelectStmt *n = makeNode(SelectStmt); + + n->targetList = $1; + $$ = (Node *) n; + } + ; + +plpgsql_strict_named_expr_list: + plpgsql_strict_named_expr + { + $$ = list_make1($1); + } + | plpgsql_strict_named_expr_list ',' plpgsql_strict_named_expr + { + $$ = lappend($1, $3); + } + ; + +plpgsql_strict_named_expr: a_expr AS ColLabel + { + $$ = makeNode(ResTarget); + $$->name = $3; + $$->indirection = NIL; + $$->val = (Node *) $1; + $$->location = @1; + } + ; /* * Name classification hierarchy. diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c index 118488c3f30..9d2dde67939 100644 --- a/src/backend/parser/parser.c +++ b/src/backend/parser/parser.c @@ -62,6 +62,12 @@ 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_STRICT_EXPR] = MODE_PLPGSQL_STRICT_EXPR, + [RAW_PARSE_PLPGSQL_STRICT_EXPR_LIST] = MODE_PLPGSQL_STRICT_EXPR_LIST, + [RAW_PARSE_PLPGSQL_STRICT_NAMED_EXPR_LIST] = MODE_PLPGSQL_STRICT_NAMED_EXPR_LIST, + [RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN1] = MODE_PLPGSQL_STRICT_EXPR_ASSIGN1, + [RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN2] = MODE_PLPGSQL_STRICT_EXPR_ASSIGN2, + [RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN3] = MODE_PLPGSQL_STRICT_EXPR_ASSIGN3, }; yyextra.have_lookahead = true; diff --git a/src/include/parser/parser.h b/src/include/parser/parser.h index be184ec5066..8e41907bd4d 100644 --- a/src/include/parser/parser.h +++ b/src/include/parser/parser.h @@ -33,6 +33,22 @@ * 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_STRICT_EXPR: parse a PL/pgSQL expression, and + * return a one-element List containing a RwaStmt node. The result is + * compatible with RAW_PARSE_PLPGSQL_EXPR, but parser allows only + * a_expr (instead almost all complete query). + * + * RAW_PARSE_PLPGSQL_STRICT_EXPR_LIST: parse a comma separated list + * of PL/pgSQL expressions (only a_expr are allowed). It is used by + * PLpGSQL CASE and OPEN commands. + * + * RAW_PARSE_PLPGSQL_STRICT_NAMED_EXPR_LIST: parse a comma separated + * list of a_expr node with labels. It is used for evaluation of + * named arguments of PLpgSQL OPEN (cursor) statement. + * + * RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGNn: parse a PL/pgSQL assignment + * statement, but only a_expr are allowed). */ typedef enum { @@ -42,6 +58,12 @@ typedef enum RAW_PARSE_PLPGSQL_ASSIGN1, RAW_PARSE_PLPGSQL_ASSIGN2, RAW_PARSE_PLPGSQL_ASSIGN3, + RAW_PARSE_PLPGSQL_STRICT_EXPR, + RAW_PARSE_PLPGSQL_STRICT_EXPR_LIST, + RAW_PARSE_PLPGSQL_STRICT_NAMED_EXPR_LIST, + RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN1, + RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN2, + RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN3, } RawParseMode; /* Values for the backslash_quote GUC */ diff --git a/src/interfaces/ecpg/preproc/parse.pl b/src/interfaces/ecpg/preproc/parse.pl index fe8d3e51780..433f4067784 100644 --- a/src/interfaces/ecpg/preproc/parse.pl +++ b/src/interfaces/ecpg/preproc/parse.pl @@ -89,7 +89,15 @@ my %replace_types = ( 'PLpgSQL_Expr' => 'ignore', 'PLAssignStmt' => 'ignore', 'plassign_target' => 'ignore', - 'plassign_equals' => 'ignore',); + 'plassign_equals' => 'ignore', + 'plpgsql_strict_expr' => 'ignore', + 'plpgsql_strict_named_expr' => 'ignore', + 'plpgsql_strict_expr_list' => 'ignore', + 'plpgsql_strict_named_expr_list' => 'ignore', + 'PLpgSQLStrictExpr' => 'ignore', + 'PLpgSQLStrictExprs' => 'ignore', + 'PLpgSQLStrictNamedExprs' => 'ignore', + 'PLAssignStmtStrictExpr' => '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/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c index f1bce708d62..8f9d3ed64d2 100644 --- a/src/pl/plpgsql/src/pl_comp.c +++ b/src/pl/plpgsql/src/pl_comp.c @@ -84,6 +84,23 @@ static const ExceptionLabelMap exception_label_map[] = { {NULL, 0} }; +static const PLpgSQL_parse_modes default_parse_modes = { + RAW_PARSE_PLPGSQL_EXPR, + RAW_PARSE_PLPGSQL_EXPR, + RAW_PARSE_PLPGSQL_EXPR, + RAW_PARSE_PLPGSQL_ASSIGN1, + RAW_PARSE_PLPGSQL_ASSIGN2, + RAW_PARSE_PLPGSQL_ASSIGN3 +}; + +static const PLpgSQL_parse_modes strict_expr_parse_modes = { + RAW_PARSE_PLPGSQL_STRICT_EXPR, + RAW_PARSE_PLPGSQL_STRICT_EXPR_LIST, + RAW_PARSE_PLPGSQL_STRICT_NAMED_EXPR_LIST, + RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN1, + RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN2, + RAW_PARSE_PLPGSQL_STRICT_EXPR_ASSIGN3 +}; /* ---------- * static prototypes @@ -355,6 +372,11 @@ do_compile(FunctionCallInfo fcinfo, function->extra_warnings = forValidator ? plpgsql_extra_warnings : 0; function->extra_errors = forValidator ? plpgsql_extra_errors : 0; + if (function->extra_errors & PLPGSQL_XCHECK_STRICTEXPRCHECK) + function->pmodes = &strict_expr_parse_modes; + else + function->pmodes = &default_parse_modes; + if (is_dml_trigger) function->fn_is_trigger = PLPGSQL_DML_TRIGGER; else if (is_event_trigger) @@ -898,6 +920,15 @@ plpgsql_compile_inline(char *proc_source) function->extra_warnings = 0; function->extra_errors = 0; + /* + * Although function->extra_errors is disabled, we want to + * do strict_expr_check inside annoymous block too. + */ + if (plpgsql_extra_errors & PLPGSQL_XCHECK_STRICTEXPRCHECK) + function->pmodes = &strict_expr_parse_modes; + else + function->pmodes = &default_parse_modes; + function->nstatements = 0; function->requires_procedure_resowner = false; diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index 8182ce28aa1..d3bf9ab6a5f 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -183,6 +183,7 @@ static void check_raise_parameters(PLpgSQL_stmt_raise *stmt); %type <expr> expr_until_semi %type <expr> expr_until_then expr_until_loop opt_expr_until_when %type <expr> opt_exitcond +%type <expr> expressions_until_then %type <var> cursor_variable %type <datum> decl_cursor_arg @@ -962,13 +963,13 @@ stmt_assign : T_DATUM switch ($1.ident ? 1 : list_length($1.idents)) { case 1: - pmode = RAW_PARSE_PLPGSQL_ASSIGN1; + pmode = plpgsql_curr_compile->pmodes->pmode_assign1; break; case 2: - pmode = RAW_PARSE_PLPGSQL_ASSIGN2; + pmode = plpgsql_curr_compile->pmodes->pmode_assign2; break; case 3: - pmode = RAW_PARSE_PLPGSQL_ASSIGN3; + pmode = plpgsql_curr_compile->pmodes->pmode_assign3; break; default: elog(ERROR, "unexpected number of names"); @@ -1244,7 +1245,7 @@ case_when_list : case_when_list case_when } ; -case_when : K_WHEN expr_until_then proc_sect +case_when : K_WHEN expressions_until_then proc_sect { PLpgSQL_case_when *new = palloc(sizeof(PLpgSQL_case_when)); @@ -1274,6 +1275,14 @@ opt_case_else : } ; +expressions_until_then : + { + $$ = read_sql_construct(K_THEN, 0, 0, "THEN", + plpgsql_curr_compile->pmodes->pmode_expr_list, + true, true, NULL, NULL); + } + ; + stmt_loop : opt_loop_label K_LOOP loop_body { PLpgSQL_stmt_loop *new; @@ -1493,7 +1502,8 @@ for_control : for_variable K_IN * Relabel first expression as an expression; * then we can check its syntax. */ - expr1->parseMode = RAW_PARSE_PLPGSQL_EXPR; + expr1->parseMode = plpgsql_curr_compile->pmodes->pmode_expr; + check_sql_expr(expr1->query, expr1->parseMode, expr1loc); @@ -1863,6 +1873,8 @@ stmt_raise : K_RAISE */ if (tok == SCONST) { + RawParseMode pmode_expr; + /* old style message and parameters */ new->message = yylval.str; /* @@ -1875,13 +1887,15 @@ stmt_raise : K_RAISE if (tok != ',' && tok != ';' && tok != K_USING) yyerror("syntax error"); + pmode_expr = plpgsql_curr_compile->pmodes->pmode_expr; + while (tok == ',') { PLpgSQL_expr *expr; expr = read_sql_construct(',', ';', K_USING, ", or ; or USING", - RAW_PARSE_PLPGSQL_EXPR, + pmode_expr, true, true, NULL, &tok); new->params = lappend(new->params, expr); @@ -2015,10 +2029,13 @@ stmt_dynexecute : K_EXECUTE PLpgSQL_stmt_dynexecute *new; PLpgSQL_expr *expr; int endtoken; + RawParseMode pmode_expr; + + pmode_expr = plpgsql_curr_compile->pmodes->pmode_expr; expr = read_sql_construct(K_INTO, K_USING, ';', "INTO or USING or ;", - RAW_PARSE_PLPGSQL_EXPR, + pmode_expr, true, true, NULL, &endtoken); @@ -2057,7 +2074,7 @@ stmt_dynexecute : K_EXECUTE { expr = read_sql_construct(',', ';', K_INTO, ", or ; or INTO", - RAW_PARSE_PLPGSQL_EXPR, + pmode_expr, true, true, NULL, &endtoken); new->params = lappend(new->params, expr); @@ -2642,7 +2659,7 @@ static PLpgSQL_expr * read_sql_expression(int until, const char *expected) { return read_sql_construct(until, 0, 0, expected, - RAW_PARSE_PLPGSQL_EXPR, + plpgsql_curr_compile->pmodes->pmode_expr, true, true, NULL, NULL); } @@ -2652,7 +2669,7 @@ read_sql_expression2(int until, int until2, const char *expected, int *endtoken) { return read_sql_construct(until, until2, 0, expected, - RAW_PARSE_PLPGSQL_EXPR, + plpgsql_curr_compile->pmodes->pmode_expr, true, true, NULL, endtoken); } @@ -3845,6 +3862,7 @@ read_cursor_args(PLpgSQL_var *cursor, int until) char **argv; StringInfoData ds; bool any_named = false; + RawParseMode pmode_expr; tok = yylex(); if (cursor->cursor_explicit_argrow < 0) @@ -3871,6 +3889,8 @@ read_cursor_args(PLpgSQL_var *cursor, int until) cursor->refname), parser_errposition(yylloc))); + pmode_expr = plpgsql_curr_compile->pmodes->pmode_expr; + /* * Read the arguments, one by one. */ @@ -3941,7 +3961,7 @@ read_cursor_args(PLpgSQL_var *cursor, int until) */ item = read_sql_construct(',', ')', 0, ",\" or \")", - RAW_PARSE_PLPGSQL_EXPR, + pmode_expr, true, true, NULL, &endtoken); @@ -3982,7 +4002,9 @@ 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 ? + plpgsql_curr_compile->pmodes->pmode_named_expr_list + : plpgsql_curr_compile->pmodes->pmode_expr_list; expr->plan = NULL; expr->paramnos = NULL; expr->target_param = -1; @@ -4156,7 +4178,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 == plpgsql_curr_compile->pmodes->pmode_expr_list); /* Do the string hacking */ initStringInfo(&ds); diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index 980f0961bc8..e7037fa8e81 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -94,6 +94,8 @@ plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source) extrachecks |= PLPGSQL_XCHECK_TOOMANYROWS; else if (pg_strcasecmp(tok, "strict_multi_assignment") == 0) extrachecks |= PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT; + else if (pg_strcasecmp(tok, "strict_expr_check") == 0) + extrachecks |= PLPGSQL_XCHECK_STRICTEXPRCHECK; else if (pg_strcasecmp(tok, "all") == 0 || pg_strcasecmp(tok, "none") == 0) { GUC_check_errdetail("Key word \"%s\" cannot be combined with other key words.", tok); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 50c3b28472b..b2fb67e8514 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -960,6 +960,21 @@ typedef enum PLpgSQL_trigtype PLPGSQL_NOT_TRIGGER, } PLpgSQL_trigtype; +/* + * Raw parse modes, that should be used for expressions, + * assignment, expression's list. When extra_errors.strict_expr_check + * is active, then only a_expr parsing is allowed. + */ +typedef struct PLpgSQL_parse_modes +{ + RawParseMode pmode_expr; + RawParseMode pmode_expr_list; + RawParseMode pmode_named_expr_list; + RawParseMode pmode_assign1; + RawParseMode pmode_assign2; + RawParseMode pmode_assign3; +} PLpgSQL_parse_modes; + /* * Complete compiled function */ @@ -1010,6 +1025,9 @@ typedef struct PLpgSQL_function unsigned int nstatements; /* counter for assigning stmtids */ bool requires_procedure_resowner; /* contains CALL or DO? */ + /* Raw parse modes configuration */ + const PLpgSQL_parse_modes *pmodes; + /* these fields change when the function is used */ struct PLpgSQL_execstate *cur_estate; unsigned long use_count; @@ -1204,6 +1222,7 @@ extern bool plpgsql_check_asserts; #define PLPGSQL_XCHECK_SHADOWVAR (1 << 1) #define PLPGSQL_XCHECK_TOOMANYROWS (1 << 2) #define PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT (1 << 3) +#define PLPGSQL_XCHECK_STRICTEXPRCHECK (1 << 4) #define PLPGSQL_XCHECK_ALL ((int) ~0) extern int plpgsql_extra_warnings; -- 2.46.0