From 73ea1474ee244d52d8f08de2b8985b2320388765 Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Tue, 19 Nov 2024 08:14:53 +0100 Subject: [PATCH 08/22] 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 1a55fa5951d..65f19598c2f 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10840,6 +10840,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 ad12e364363..fe3f1cca006 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5411,6 +5411,21 @@ SELECT current_user_id; 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 56a8fe5d74f..37b91e50794 100644 --- a/src/backend/nodes/nodeFuncs.c +++ b/src/backend/nodes/nodeFuncs.c @@ -1662,6 +1662,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; @@ -4681,6 +4684,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 7e7158ac099..674e8af256c 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -517,7 +517,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 @@ -874,7 +874,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 '*' '/' '%' @@ -15593,6 +15593,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 @@ -16979,6 +16992,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 4e78529c40d..b85a97ca78e 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 93915031be8..300ea348788 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 8d3d3c9c757..ebb41e3b5e8 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -8630,8 +8630,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 7947a82d1f6..4f96788d1db 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1614,6 +1614,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 d18a17544a6..47041cbe80b 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -717,6 +717,7 @@ #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 93241bd55b8..f88edc0d418 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -312,6 +312,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 624858db301..1fa21c4e179 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -388,6 +388,8 @@ typedef struct Param Oid paramvarid; /* true when param is used like base node of assignment indirection */ 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 b551bd08b5d..174f9140dcf 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1937,3 +1937,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 55cc3cccc32..172918b603c 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -1335,3 +1335,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.47.1