From 45f092d71c962a8213c97458537a1eda859779db Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Tue, 19 Nov 2024 08:14:53 +0100 Subject: [PATCH 08/21] variable fence syntax support and variable fence usage guard support this patch introduces a concept of variable fence - syntax for variable reference `VARIABLE(varname)` that is not in collision with column reference. When variable fence usage guard warning is active, then usage variable without variable fence in the case, where there can be column references, the the warning is raised. initial implementation of variable fence --- doc/src/sgml/config.sgml | 65 ++++++++++++++++ doc/src/sgml/ddl.sgml | 15 ++++ src/backend/nodes/nodeFuncs.c | 6 ++ src/backend/parser/gram.y | 28 ++++++- src/backend/parser/parse_expr.c | 74 ++++++++++++++++++- src/backend/parser/parse_target.c | 7 ++ src/backend/utils/adt/ruleutils.c | 12 ++- src/backend/utils/misc/guc_tables.c | 9 +++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/nodes/parsenodes.h | 10 +++ src/include/nodes/primnodes.h | 2 + src/include/parser/parse_expr.h | 1 + .../regress/expected/session_variables.out | 59 +++++++++++++++ src/test/regress/sql/session_variables.sql | 41 ++++++++++ 14 files changed, 323 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 4e416cc5892..b16e081a242 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -11186,6 +11186,71 @@ DETAIL: Session variables can be shadowed by columns, routine's variables and r + + session_variables_use_fence_warning_guard (boolean) + + session_variables_use_fence_warning_guard configuration parameter + + + + + When on, a warning is raised when a session variable identifier is used + inside a query without variable fence. The default is off. + The warning is raised only when variable is used in places, where an + collisions with column names is possible. + +CREATE TABLE foo(a int); +INSERT INTO foo VALUES(10); +CREATE VARIABLE b int; +LET b = 100; +SELECT a, b FROM foo; + + + + a | b +----+----- + 10 | 100 +(1 row) + + + +SET session_variables_use_fence_warning_guard TO on; +SELECT a, b FROM foo; + + + +WARNING: session variable "b" is not used inside variable fence +LINE 1: SELECT a, b FROM foo; + ^ +DETAIL: The collision of session variable' names and column names is possible. + a | b +----+----- + 10 | 100 +(1 row) + + + +SELECT a, VARIABLE(b) FROM foo; + + + + a | b +----+----- + 10 | 100 +(1 row) + + + + + This feature can significantly increase log size, so it's disabled by + default. Unless another collision resolution technique is used + (dedicated schema or using prefixes like _), + the use of variable fence syntax is strongly recommended, and this + warning should be enabled. + + + + standard_conforming_strings (boolean) stringsstandard conforming diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 6c1ca4a95fd..d6f39508c79 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5454,6 +5454,21 @@ SELECT name FROM foo; schema.variable. It is strongly recommended to rename shadowed variables or use qualified names always. + + Variable fence is special syntax for session + variable identifier. Only name or qualified name can be used inside the + variable fence, and this name is used as only session variable identifier. + +SELECT VARIABLE(current_user_id); + + + + + When there is a risk of possible collisions between variable identifiers + and column names, then using variable fence syntax can be recommended. + Usage session variable without variable fence can be detected by + warning . + diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c index d8fc3d9fa69..575365eefcd 100644 --- a/src/backend/nodes/nodeFuncs.c +++ b/src/backend/nodes/nodeFuncs.c @@ -1673,6 +1673,9 @@ exprLocation(const Node *expr) case T_ParamRef: loc = ((const ParamRef *) expr)->location; break; + case T_VariableFence: + loc = ((const VariableFence *) expr)->location; + break; case T_A_Const: loc = ((const A_Const *) expr)->location; break; @@ -4715,6 +4718,9 @@ raw_expression_tree_walker_impl(Node *node, return true; } break; + case T_VariableFence: + /* we assume the fields contain nothing interesting */ + break; default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node)); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 81ae5274ec6..9afb1048b14 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -525,7 +525,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type def_arg columnElem where_clause where_or_current_clause a_expr b_expr c_expr AexprConst indirection_el opt_slice_bound columnref in_expr having_clause func_table xmltable array_expr - OptWhereClause operator_def_arg + OptWhereClause operator_def_arg variable_fence %type opt_column_and_period_list %type rowsfrom_item rowsfrom_list opt_col_def_list %type opt_ordinality opt_without_overlaps @@ -882,7 +882,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); */ %nonassoc UNBOUNDED NESTED /* ideally would have same precedence as IDENT */ %nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP - SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH + SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH VARIABLE %left Op OPERATOR /* multi-character ops and user-defined operators */ %left '+' '-' %left '*' '/' '%' @@ -15696,6 +15696,19 @@ c_expr: columnref { $$ = $1; } else $$ = $2; } + | variable_fence opt_indirection + { + if ($2) + { + A_Indirection *n = makeNode(A_Indirection); + + n->arg = (Node *) $1; + n->indirection = check_indirection($2, yyscanner); + $$ = (Node *) n; + } + else + $$ = $1; + } | case_expr { $$ = $1; } | func_expr @@ -17082,6 +17095,17 @@ case_arg: a_expr { $$ = $1; } | /*EMPTY*/ { $$ = NULL; } ; +variable_fence: + VARIABLE '(' any_name ')' + { + VariableFence *vf = makeNode(VariableFence); + + vf->varname = $3; + vf->location = @3; + $$ = (Node *) vf; + } + ; + columnref: ColId { $$ = makeColumnRef($1, NIL, @1, yyscanner); diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index dccc5030438..a439a156216 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; +bool session_variables_use_fence_warning_guard = false; static Node *transformExprRecurse(ParseState *pstate, Node *expr); @@ -81,6 +82,7 @@ static Node *transformWholeRowRef(ParseState *pstate, static Node *transformIndirection(ParseState *pstate, A_Indirection *ind); static Node *transformTypeCast(ParseState *pstate, TypeCast *tc); static Node *transformCollateClause(ParseState *pstate, CollateClause *c); +static Node *transformVariableFence(ParseState *pstate, VariableFence *vf); static Node *transformJsonObjectConstructor(ParseState *pstate, JsonObjectConstructor *ctor); static Node *transformJsonArrayConstructor(ParseState *pstate, @@ -112,7 +114,7 @@ static Node *make_nulltest_from_distinct(ParseState *pstate, A_Expr *distincta, Node *arg); static Node *makeParamSessionVariable(ParseState *pstate, Oid varid, Oid typid, int32 typmod, Oid collid, - char *attrname, int location); + char *attrname, bool fenced, int location); /* @@ -377,6 +379,10 @@ transformExprRecurse(ParseState *pstate, Node *expr) result = transformJsonFuncExpr(pstate, (JsonFuncExpr *) expr); break; + case T_VariableFence: + result = transformVariableFence(pstate, (VariableFence *) expr); + break; + default: /* should not reach here */ elog(ERROR, "unrecognized node type: %d", (int) nodeTag(expr)); @@ -1028,7 +1034,20 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) node = makeParamSessionVariable(pstate, varid, typid, typmod, collid, - attrname, cref->location); + attrname, false, cref->location); + + /* + * The variable is not inside variable's fence, so raise warning + * when variable fence guard is active and the query has FROM + * clause. + */ + if (session_variables_use_fence_warning_guard && pstate->p_rtable) + ereport(WARNING, + (errcode(ERRCODE_AMBIGUOUS_COLUMN), + errmsg("session variable \"%s\" is not used inside variable fence", + NameListToString(cref->fields)), + errdetail("The collision of session variable' names and column names is possible."), + parser_errposition(pstate, cref->location))); } } } @@ -1071,7 +1090,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) static Node * makeParamSessionVariable(ParseState *pstate, Oid varid, Oid typid, int32 typmod, Oid collid, - char *attrname, int location) + char *attrname, bool fenced, int location) { Param *param; @@ -1079,6 +1098,7 @@ makeParamSessionVariable(ParseState *pstate, param->paramkind = PARAM_VARIABLE; param->paramvarid = varid; + param->paramvarfenced = fenced; param->paramtype = typid; param->paramtypmod = typmod; param->paramcollid = collid; @@ -1157,6 +1177,54 @@ transformParamRef(ParseState *pstate, ParamRef *pref) return result; } +static Node * +transformVariableFence(ParseState *pstate, VariableFence *vf) +{ + Node *result; + Oid varid = InvalidOid; + char *attrname = NULL; + bool not_unique; + + /* VariableFence can be used only in context when variables are supported */ + if (!expr_kind_allows_session_variables(pstate->p_expr_kind)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("session variable reference is not supported here"), + parser_errposition(pstate, vf->location))); + + /* takes an AccessShareLock on the session variable */ + varid = IdentifyVariable(vf->varname, &attrname, ¬_unique, false); + + if (not_unique) + ereport(ERROR, + (errcode(ERRCODE_AMBIGUOUS_PARAMETER), + errmsg("session variable reference \"%s\" is ambiguous", + NameListToString(vf->varname)), + parser_errposition(pstate, vf->location))); + + if (OidIsValid(varid)) + { + Oid typid; + int32 typmod; + Oid collid; + + get_session_variable_type_typmod_collid(varid, &typid, &typmod, + &collid); + + result = makeParamSessionVariable(pstate, + varid, typid, typmod, collid, + attrname, true, vf->location); + } + else + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("session variable \"%s\" doesn't exist", + NameListToString(vf->varname)), + parser_errposition(pstate, vf->location))); + + return result; +} + /* Test whether an a_expr is a plain NULL constant or not */ static bool exprIsNullConstant(Node *arg) diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c index 4aba0d9d4d5..cf01bff65a9 100644 --- a/src/backend/parser/parse_target.c +++ b/src/backend/parser/parse_target.c @@ -2034,6 +2034,13 @@ FigureColnameInternal(Node *node, char **name) (int) ((JsonFuncExpr *) node)->op); } break; + case T_VariableFence: + { + /* return last field name */ + *name = strVal(llast(((VariableFence *) node)->varname)); + return 2; + } + break; default: break; } diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index d6bdae9af7d..e0e15d1c64c 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -8734,8 +8734,16 @@ get_parameter(Param *param, deparse_context *context) /* translate paramvarid to session variable name */ if (param->paramkind == PARAM_VARIABLE) { - appendStringInfo(context->buf, "%s", - generate_session_variable_name(param->paramvarid)); + if (param->paramvarfenced) + { + appendStringInfo(context->buf, "VARIABLE(%s)", + generate_session_variable_name(param->paramvarid)); + } + else + { + appendStringInfo(context->buf, "%s", + generate_session_variable_name(param->paramvarid)); + } return; } diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index fce97e4facd..ffe711d72a7 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1637,6 +1637,15 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, + { + {"session_variables_use_fence_warning_guard", PGC_USERSET, CLIENT_CONN_STATEMENT, + gettext_noop("Raise a warning when variable is not used inside variable fence."), + NULL + }, + &session_variables_use_fence_warning_guard, + 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 85171c5ea23..ee8bb111572 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -743,6 +743,7 @@ autovacuum_worker_slots = 16 # autovacuum worker slots to allocate #default_transaction_deferrable = off #session_replication_role = 'origin' #session_variables_ambiguity_warning = off +#session_variables_use_fence_warning_guard = 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/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 33d03c8f174..7d7ed1b8d9d 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -321,6 +321,16 @@ typedef struct ParamRef ParseLoc location; /* token location, or -1 if unknown */ } ParamRef; +/* + * VariableFence - ensure so fields will be interpretted as a variable + */ +typedef struct VariableFence +{ + NodeTag type; + List *varname; /* variable name (String nodes) */ + ParseLoc location; /* token location, or -1 if unknown */ +} VariableFence; + /* * A_Expr - infix, prefix, and postfix expressions */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 24033045746..be63a8d0645 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -411,6 +411,8 @@ typedef struct Param * param is used just for execution of UPDATE operation. */ bool parambasenode; + /* true when variable is used inside an fence */ + bool paramvarfenced; /* token location, or -1 if unknown */ ParseLoc location; } Param; diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h index d2209070181..5bde43758d1 100644 --- a/src/include/parser/parse_expr.h +++ b/src/include/parser/parse_expr.h @@ -18,6 +18,7 @@ /* GUC parameters */ extern PGDLLIMPORT bool Transform_null_equals; extern PGDLLIMPORT bool session_variables_ambiguity_warning; +extern PGDLLIMPORT bool session_variables_use_fence_warning_guard; extern Node *transformExpr(ParseState *pstate, Node *expr, ParseExprKind exprKind); diff --git a/src/test/regress/expected/session_variables.out b/src/test/regress/expected/session_variables.out index 7c350ad5ef9..9ac08a14e0b 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1969,3 +1969,62 @@ SET session_variables_ambiguity_warning TO off; DROP TABLE public.xxtab; DROP SCHEMA xxtab CASCADE; NOTICE: drop cascades to session variable xxtab.avar +-- test session_variables_use_fence_warning_guard +SET session_variables_ambiguity_warning TO on; +SET session_variables_use_fence_warning_guard TO on; +CREATE SCHEMA testvar; +SET search_path TO 'testvar'; +CREATE VARIABLE a AS int; +LET a = 10; +CREATE TABLE test_table(a int, b int); +INSERT INTO test_table VALUES(20, 20); +-- no warning +SELECT a; + a +---- + 10 +(1 row) + +-- warning variable is shadowed +SELECT a, b FROM test_table; +WARNING: session variable "a" is shadowed +LINE 1: SELECT a, b FROM test_table; + ^ +DETAIL: Session variables can be shadowed by columns, routine's variables and routine's arguments with the same name. + a | b +----+---- + 20 | 20 +(1 row) + +-- no warning +SELECT variable(a) FROM test_table; + a +---- + 10 +(1 row) + +ALTER TABLE test_table DROP COLUMN a; +-- warning - variable fence is not used +SELECT a, b FROM test_table; +WARNING: session variable "a" is not used inside variable fence +LINE 1: SELECT a, b FROM test_table; + ^ +DETAIL: The collision of session variable' names and column names is possible. + a | b +----+---- + 10 | 20 +(1 row) + +-- no warning +SELECT variable(a), b FROM test_table; + a | b +----+---- + 10 | 20 +(1 row) + +DROP VARIABLE a; +DROP TABLE test_table; +DROP SCHEMA testvar; +SET session_variables_ambiguity_warning TO DEFAULT; +SET session_variables_use_fence_warning_guard TO DEFAULT; +SET search_path TO DEFAULT; diff --git a/src/test/regress/sql/session_variables.sql b/src/test/regress/sql/session_variables.sql index 68be415bf26..c5420183d94 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -1360,3 +1360,44 @@ SET session_variables_ambiguity_warning TO off; DROP TABLE public.xxtab; DROP SCHEMA xxtab CASCADE; + +-- test session_variables_use_fence_warning_guard +SET session_variables_ambiguity_warning TO on; +SET session_variables_use_fence_warning_guard TO on; + +CREATE SCHEMA testvar; + +SET search_path TO 'testvar'; + +CREATE VARIABLE a AS int; +LET a = 10; + +CREATE TABLE test_table(a int, b int); + +INSERT INTO test_table VALUES(20, 20); + +-- no warning +SELECT a; + +-- warning variable is shadowed +SELECT a, b FROM test_table; + +-- no warning +SELECT variable(a) FROM test_table; + +ALTER TABLE test_table DROP COLUMN a; + +-- warning - variable fence is not used +SELECT a, b FROM test_table; + +-- no warning +SELECT variable(a), b FROM test_table; + +DROP VARIABLE a; +DROP TABLE test_table; + +DROP SCHEMA testvar; + +SET session_variables_ambiguity_warning TO DEFAULT; +SET session_variables_use_fence_warning_guard TO DEFAULT; +SET search_path TO DEFAULT; -- 2.48.1