From 5f42f7b7fe8e38ff48c01a1c8f51bece5c008c8f Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Tue, 21 May 2024 17:55:24 +0200 Subject: [PATCH 07/20] 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 | 6 +- src/backend/parser/parse_expr.c | 48 +++++++++++++-- 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 | 22 +++++++ src/test/regress/sql/session_variables.sql | 20 +++++++ 10 files changed, 224 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 3dec0b7cfeb..9f14ad36f82 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10685,6 +10685,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 92730799f40..9d5082dd3ce 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5353,7 +5353,11 @@ SELECT current_user_id; The session variables can be shadowed by column references in a query. 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. + 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. diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 73be2a8e10a..f0ebbcf5beb 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -47,6 +47,7 @@ /* GUC parameters */ bool Transform_null_equals = false; +bool session_variables_ambiguity_warning = false; static Node *transformExprRecurse(ParseState *pstate, Node *expr); static Node *transformParamRef(ParseState *pstate, ParamRef *pref); @@ -943,7 +944,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) * question is if we want to identify session's variables in these * contexts? The code can be more simple, when we don't do it, but then we * cannot to raise maybe useful message like "you cannot to use session - * variables here". + * variables here". On second hand, in this case the warnings about + * session's variable shadowing can be messy. */ if (expr_kind_allows_session_variables(pstate->p_expr_kind)) { @@ -962,10 +964,48 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) * SELECT foo.a, foo.b, foo.c FROM foo; * * This case can be messy and then we disallow it. When we know that a - * possible variable will be shadowed, we don't try to identify - * variable. + * possible variable will be shadowed, we try to identify variable + * only when session_variables_ambiguity_warning is requested. */ - 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 { /* * The AccessShareLock is created on related session variable. The diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 630ed0f1629..0d5de40880f 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1544,6 +1544,15 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, + { + {"session_variables_ambiguity_warning", PGC_USERSET, CLIENT_CONN_OTHER, + 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 9ec9f97e926..ebf6f016d2d 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -713,6 +713,7 @@ #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 9b46dfd9ecc..0d64b300f8a 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 6e8b4134c06..ec07f1cbae9 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 398e7fe2a7c..ef705a4cc87 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 fca21f4137d..2f903243767 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1248,3 +1248,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 5186cd9dc9f..a5d3af6ade1 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -841,3 +841,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.45.2