From 33ff33fc8546600adf9a8267be110b5a5dd88124 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 | 10 ++++ 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 | 22 +++++++ src/test/regress/sql/session_variables.sql | 20 +++++++ 10 files changed, 230 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 0aec11f4432..7578b078623 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10706,6 +10706,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 7eee2a8e803..06328352ec1 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5321,6 +5321,16 @@ SELECT current_user_id; the schema name, columns can use table aliases, routine variables can use block labels, and routine arguments can use the routine name. + + + 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. + diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 8760876c686..468dc03e275 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); @@ -962,11 +963,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 686309db58b..b68bbe94fec 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1595,6 +1595,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 667e0dc40a2..3eae1c15160 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 7e4d41de3e6..660423d5de8 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1258,3 +1258,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 b3f3d6f16a1..391c1349ffa 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -853,3 +853,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.46.1