From 69df1c00c8fea33a171b2b72fc0d275b6e5f3d70 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 | 57 ++++++++++++++ src/test/regress/sql/session_variables.sql | 36 +++++++++ 14 files changed, 316 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index ed5a7adc84..872b3ed849 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -10838,6 +10838,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 55245c1bcd..9f231653fd 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -5395,6 +5395,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 48d86ee720..3ef8bc84e0 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 aad47305f2..fbf06f1149 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -518,7 +518,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 @@ -875,7 +875,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 '*' '/' '%' @@ -15603,6 +15603,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 @@ -16989,6 +17002,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 3602c3572d..d094ac3013 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; @@ -1154,6 +1174,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 76bf88c3ca..cc63b44bba 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 4932d87d83..7b97e1749c 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -8628,8 +8628,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 0f1806d2f5..5881b0d5d4 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_OTHER, + 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 d18a17544a..47041cbe80 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 04ab22bd49..01bc31e8a9 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 4b2424d6d3..053af6ae09 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -386,6 +386,8 @@ typedef struct Param Oid paramcollid pg_node_attr(query_jumble_ignore); /* OID of session variable if it is used */ Oid paramvarid; + /* 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 0d64b300f8..0948a0b1b7 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 317fd073fc..cb1e0215d3 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1314,3 +1314,60 @@ 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; +SET search_path TO 'public'; +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; +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 9a70c8b1f1..74865f43f6 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -909,3 +909,39 @@ 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; +SET search_path TO 'public'; + +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; + +SET session_variables_ambiguity_warning TO DEFAULT; +SET session_variables_use_fence_warning_guard TO DEFAULT; +SET search_path TO DEFAULT; -- 2.47.1