From 7397abc6e620bb86da68d7588de392c14bf30d58 Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Tue, 21 May 2024 17:55:24 +0200 Subject: [PATCH 07/21] 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 | 22 +++++++ src/test/regress/sql/session_variables.sql | 20 +++++++ 10 files changed, 231 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 76ab72db964..02f7f0e1240 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10754,6 +10754,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 327548fae12..8f4da08ef86 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5359,6 +5359,17 @@ 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. 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 aba5259e027..3602c3572d7 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 9845abd6932..005f961edbd 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1605,6 +1605,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 407cd1e08ca..f60ee5d2c44 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -715,6 +715,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 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 85771032de3..50fb478b5e9 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1261,3 +1261,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 d8397573eeb..e7a4fdedae4 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -854,3 +854,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.47.0