From adff6db1c2c31e5849d065acff32ad689f49657a Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Sat, 20 Jan 2024 14:03:09 +0100 Subject: [PATCH 16/19] 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 | 10 ++++ 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 +++++++ 9 files changed, 224 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 65a6e6c408..b6a8feeaf5 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10664,6 +10664,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 3d21cef92e..db9b9d5bf8 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5324,7 +5324,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 + variable.column. diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 334ad7a431..816d64155b 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -46,6 +46,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); @@ -924,7 +925,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)) { @@ -943,10 +945,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 d77214795d..ebe73138c9 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1535,6 +1535,16 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, + { + {"session_variables_ambiguity_warning", PGC_USERSET, DEVELOPER_OPTIONS, + gettext_noop("Raise warning when reference to a session variable is ambiguous."), + NULL, + GUC_NOT_IN_SAMPLE + }, + &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/include/parser/parse_expr.h b/src/include/parser/parse_expr.h index 9b46dfd9ec..0d64b300f8 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 1a2aeba24d..789c3123b0 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_session_variable.out +++ b/src/pl/plpgsql/src/expected/plpgsql_session_variable.out @@ -353,6 +353,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 398e7fe2a7..ef705a4cc8 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 421194e49e..10ec9107d8 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1710,3 +1710,25 @@ SELECT var1; LET var1 = 30; ERROR: session variable "public.var1" is declared IMMUTABLE DROP VARIABLE var1; +-- 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 dce4bd057f..e011bdb6ed 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -1162,3 +1162,23 @@ SELECT var1; LET var1 = 30; DROP VARIABLE var1; + +-- 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.44.0