From 88eaaaeb03bc2a63ddd67f7a0bb9fb45576d8394 Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Tue, 21 May 2024 17:55:24 +0200 Subject: [PATCH 07/22] GUC session_variables_ambiguity_warning Inside an query the session variables can be shadowed. This behaviour is by design, because this ensuring so new variables doesn't break existing queries. But this behaviour can be confusing, because shadowing is quiet. When new GUC session_variables_ambiguity_warning is on, then the warning is displayed, when some session variable can be used, but it is not used due shadowing. This feature should to help with investigation of unexpected behaviour. Note: PLpgSQL has an option that controls priority of identifier's spaces (SQL or PLpgSQL). Default behaviour doesn't allows collisions. I believe this default is the best. I cannot to implement similar very strict behaviour to session variables, because one unhappy named session variable can breaks an applications. The problems related to badly named PLpgSQL's varible is limitted just to only one routine. --- doc/src/sgml/config.sgml | 60 +++++++++++++++++++ doc/src/sgml/ddl.sgml | 11 ++++ src/backend/parser/parse_expr.c | 49 +++++++++++++-- src/backend/utils/misc/guc_tables.c | 9 +++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/parser/parse_expr.h | 1 + .../src/expected/plpgsql_session_variable.out | 33 ++++++++++ .../src/sql/plpgsql_session_variable.sql | 29 +++++++++ .../regress/expected/session_variables.out | 26 ++++++++ src/test/regress/sql/session_variables.sql | 26 ++++++++ 10 files changed, 241 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 38244409e3c..6004294015f 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10868,6 +10868,66 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' + + session_variables_ambiguity_warning (boolean) + + session_variables_ambiguity_warning configuration parameter + + + + + When on, a warning is raised when any identifier in a query could be + used as both a column identifier, routine variable or a session + variable identifier. The default is off. + + + Session variables can be shadowed by column references in a query, this + is an expected behavior. Previously working queries shouldn't error out + by creating any session variable, so session variables are always shadowed + if an identifier is ambiguous. Variables should be referenced using + anunambiguous identifier without any possibility for a collision with + identifier of other database objects (column names or record fields names). + The warning messages emitted when enabling session_variables_ambiguity_warning + can help finding such identifier collision. + +CREATE TABLE foo(a int); +INSERT INTO foo VALUES(10); +CREATE VARIABLE a int; +LET a = 100; +SELECT a FROM foo; + + + + a +---- + 10 +(1 row) + + + +SET session_variables_ambiguity_warning TO on; +SELECT a FROM foo; + + + +WARNING: session variable "a" is shadowed +LINE 1: SELECT a FROM foo; + ^ +DETAIL: Session variables can be shadowed by columns, routine's variables and routine's arguments with the same name. + a +---- + 10 +(1 row) + + + + This feature can significantly increase log size, so it's disabled by + default. For testing or development environments it's recommended to + enable it if you use session variables. + + + + standard_conforming_strings (boolean) stringsstandard conforming diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 5e671d5be4d..96912aaa436 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5445,6 +5445,17 @@ SELECT name FROM foo; Alice + + + When a query contains identifiers or qualified identifiers that could be + used as both a session variable identifiers and as column identifier, + then the column identifier is preferred every time. Warnings can be + emitted when this situation happens by enabling configuration parameter . User can explicitly + qualify the source object by syntax table.column or + schema.variable. It is strongly recommended to rename + shadowed variables or use qualified names always. + diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 4f208c7efdd..63f1732f69e 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -45,6 +45,7 @@ /* GUC parameters */ bool Transform_null_equals = false; +bool session_variables_ambiguity_warning = false; static Node *transformExprRecurse(ParseState *pstate, Node *expr); @@ -960,11 +961,51 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) * * SELECT foo.a, foo.b, foo.c FROM foo; * - * However, that is very confusing, so we disallow it. We don't try to - * identify a variable if we know that it would be shadowed. - * ----- + * However, that is very confusing, so we disallow it. + * + * When session_variables_ambiguity_warning is requested, then we + * need to identify a variable although we know, so this variable + * would be shadowed. */ - if (!node && !(relname && crerr == CRERR_NO_COLUMN)) + if (node || (relname && crerr == CRERR_NO_COLUMN)) + { + /* + * In this path we just try (if it is wanted) detect if session + * variable is shadowed. + */ + if (session_variables_ambiguity_warning) + { + /* + * The AccessShareLock is created on related session variable. + * The lock will be kept for the whole transaction. + */ + varid = IdentifyVariable(cref->fields, &attrname, ¬_unique, true); + + if (OidIsValid(varid)) + { + /* this path will ending by WARNING. Unlock variable first */ + UnlockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock); + + if (node) + ereport(WARNING, + (errcode(ERRCODE_AMBIGUOUS_COLUMN), + errmsg("session variable \"%s\" is shadowed", + NameListToString(cref->fields)), + errdetail("Session variables can be shadowed by columns, routine's variables and routine's arguments with the same name."), + parser_errposition(pstate, cref->location))); + else + /* session variable is shadowed by RTE */ + ereport(WARNING, + (errcode(ERRCODE_AMBIGUOUS_COLUMN), + errmsg("session variable \"%s.%s\" is shadowed", + get_namespace_name(get_session_variable_namespace(varid)), + get_session_variable_name(varid)), + errdetail("Session variables can be shadowed by tables or table's aliases with the same name."), + parser_errposition(pstate, cref->location))); + } + } + } + else { /* takes an AccessShareLock on the session variable */ varid = IdentifyVariable(cref->fields, &attrname, ¬_unique, false); diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index ce7534d4d23..1147169f768 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1606,6 +1606,15 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, + { + {"session_variables_ambiguity_warning", PGC_USERSET, CLIENT_CONN_STATEMENT, + gettext_noop("Raise a warning when reference to a session variable is ambiguous."), + NULL + }, + &session_variables_ambiguity_warning, + false, + NULL, NULL, NULL + }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default read-only status of new transactions."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index c40b7a3121e..8fbbd5a533f 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -730,6 +730,7 @@ autovacuum_worker_slots = 16 # autovacuum worker slots to allocate #default_transaction_read_only = off #default_transaction_deferrable = off #session_replication_role = 'origin' +#session_variables_ambiguity_warning = off #statement_timeout = 0 # in milliseconds, 0 is disabled #transaction_timeout = 0 # in milliseconds, 0 is disabled #lock_timeout = 0 # in milliseconds, 0 is disabled diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h index efbaff8e710..d2209070181 100644 --- a/src/include/parser/parse_expr.h +++ b/src/include/parser/parse_expr.h @@ -17,6 +17,7 @@ /* GUC parameters */ extern PGDLLIMPORT bool Transform_null_equals; +extern PGDLLIMPORT bool session_variables_ambiguity_warning; extern Node *transformExpr(ParseState *pstate, Node *expr, ParseExprKind exprKind); diff --git a/src/pl/plpgsql/src/expected/plpgsql_session_variable.out b/src/pl/plpgsql/src/expected/plpgsql_session_variable.out index ab779900596..a6523f7afe2 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_session_variable.out +++ b/src/pl/plpgsql/src/expected/plpgsql_session_variable.out @@ -354,6 +354,39 @@ $$; NOTICE: session variable is 100 NOTICE: plpgsql variable is 1000 NOTICE: variable is 1000 +-- againt with session_variables_ambiguity_warning(on) +SET session_variables_ambiguity_warning TO on; +DO $$ +<> +DECLARE plpgsql_sv_var1 int; +BEGIN + LET plpgsql_sv_var1 = 100; + + -- should be ok without warning + plpgsql_sv_var1 := 1000; + + -- should be ok without warning + -- print 100; + RAISE NOTICE 'session variable is %', public.plpgsql_sv_var1; + + -- should be ok without warning + -- print 1000 + RAISE NOTICE 'plpgsql variable is %', myblock.plpgsql_sv_var1; + + -- should to print plpgsql variable with warning + -- print 1000 + RAISE NOTICE 'variable is %', plpgsql_sv_var1; +END; +$$; +NOTICE: session variable is 100 +NOTICE: plpgsql variable is 1000 +WARNING: session variable "plpgsql_sv_var1" is shadowed +LINE 1: plpgsql_sv_var1 + ^ +DETAIL: Session variables can be shadowed by columns, routine's variables and routine's arguments with the same name. +QUERY: plpgsql_sv_var1 +NOTICE: variable is 1000 +SET session_variables_ambiguity_warning TO off; DROP VARIABLE plpgsql_sv_var1; -- the value should not be corrupted CREATE VARIABLE plpgsql_sv_v text; diff --git a/src/pl/plpgsql/src/sql/plpgsql_session_variable.sql b/src/pl/plpgsql/src/sql/plpgsql_session_variable.sql index a3cc264cf38..9b8e90e81fa 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_session_variable.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_session_variable.sql @@ -260,6 +260,35 @@ BEGIN END; $$; +-- againt with session_variables_ambiguity_warning(on) + +SET session_variables_ambiguity_warning TO on; + +DO $$ +<> +DECLARE plpgsql_sv_var1 int; +BEGIN + LET plpgsql_sv_var1 = 100; + + -- should be ok without warning + plpgsql_sv_var1 := 1000; + + -- should be ok without warning + -- print 100; + RAISE NOTICE 'session variable is %', public.plpgsql_sv_var1; + + -- should be ok without warning + -- print 1000 + RAISE NOTICE 'plpgsql variable is %', myblock.plpgsql_sv_var1; + + -- should to print plpgsql variable with warning + -- print 1000 + RAISE NOTICE 'variable is %', plpgsql_sv_var1; +END; +$$; + +SET session_variables_ambiguity_warning TO off; + DROP VARIABLE plpgsql_sv_var1; -- the value should not be corrupted diff --git a/src/test/regress/expected/session_variables.out b/src/test/regress/expected/session_variables.out index 2783caa8446..7c350ad5ef9 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1524,6 +1524,9 @@ CREATE TABLE vartest_foo(a int, b int); INSERT INTO vartest_foo VALUES(10,20), (30,40); CREATE VARIABLE var1 AS vartest_foo; LET var1 = (100,200); +-- no variable is shadowed here +-- no warning expected +SET session_variables_ambiguity_warning TO on; -- should to fail SELECT var1.* FROM vartest_foo; ERROR: missing FROM-clause entry for table "var1" @@ -1549,6 +1552,7 @@ SELECT count(var1.*) FROM vartest_foo var1; 2 (1 row) +SET session_variables_ambiguity_warning TO off; SET SEARCH_PATH TO DEFAULT; DROP SCHEMA vartest CASCADE; NOTICE: drop cascades to 2 other objects @@ -1943,3 +1947,25 @@ SELECT var1; (1 row) DROP VARIABLE var1, var2; +-- test session_variables_ambiguity_warning +CREATE SCHEMA xxtab; +CREATE VARIABLE xxtab.avar int; +CREATE TABLE public.xxtab(avar int); +INSERT INTO public.xxtab VALUES(1); +LET xxtab.avar = 20; +SET session_variables_ambiguity_warning TO on; +--- should to raise warning, show 1 +SELECT xxtab.avar FROM public.xxtab; +WARNING: session variable "xxtab.avar" is shadowed +LINE 1: SELECT xxtab.avar FROM public.xxtab; + ^ +DETAIL: Session variables can be shadowed by columns, routine's variables and routine's arguments with the same name. + avar +------ + 1 +(1 row) + +SET session_variables_ambiguity_warning TO off; +DROP TABLE public.xxtab; +DROP SCHEMA xxtab CASCADE; +NOTICE: drop cascades to session variable xxtab.avar diff --git a/src/test/regress/sql/session_variables.sql b/src/test/regress/sql/session_variables.sql index a624ab8b67f..68be415bf26 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -1079,6 +1079,10 @@ INSERT INTO vartest_foo VALUES(10,20), (30,40); CREATE VARIABLE var1 AS vartest_foo; LET var1 = (100,200); +-- no variable is shadowed here +-- no warning expected +SET session_variables_ambiguity_warning TO on; + -- should to fail SELECT var1.* FROM vartest_foo; @@ -1091,6 +1095,8 @@ SELECT count(var1.*) FROM vartest_foo; -- should be ok SELECT count(var1.*) FROM vartest_foo var1; +SET session_variables_ambiguity_warning TO off; + SET SEARCH_PATH TO DEFAULT; DROP SCHEMA vartest CASCADE; @@ -1334,3 +1340,23 @@ BEGIN; DROP VARIABLE var1; ROLLBACK; SELECT var1; DROP VARIABLE var1, var2; + +-- test session_variables_ambiguity_warning +CREATE SCHEMA xxtab; + +CREATE VARIABLE xxtab.avar int; + +CREATE TABLE public.xxtab(avar int); + +INSERT INTO public.xxtab VALUES(1); + +LET xxtab.avar = 20; + +SET session_variables_ambiguity_warning TO on; +--- should to raise warning, show 1 +SELECT xxtab.avar FROM public.xxtab; + +SET session_variables_ambiguity_warning TO off; + +DROP TABLE public.xxtab; +DROP SCHEMA xxtab CASCADE; -- 2.48.1