From eaec2f9018e5e486f92138f6096bf32276bd9a87 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sat, 5 Nov 2022 10:22:25 +0100 Subject: [PATCH 04/15] review comments --- src/backend/access/transam/xact.c | 1 + src/backend/executor/execParallel.c | 3 +-- src/backend/optimizer/util/clauses.c | 1 - src/backend/parser/analyze.c | 6 ++---- src/backend/utils/cache/plancache.c | 3 +++ src/backend/utils/misc/guc_tables.c | 2 +- src/include/catalog/pg_proc.dat | 2 ++ src/include/commands/session_variable.h | 1 + src/include/nodes/pathnodes.h | 5 +++-- src/include/nodes/plannodes.h | 2 +- src/include/nodes/primnodes.h | 3 +-- 11 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 8b33d336142..45a8f237f30 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2217,6 +2217,7 @@ CommitTransaction(void) /* Let ON COMMIT DROP or ON TRANSACTION END */ /* review: everything else is called just AtEOXact_. Also why suffix * _on_xact_actions? Seems redundant with AtEOXact_ prefix. */ + /* review: seems like unnecessary comment change? */ AtPreEOXact_SessionVariable_on_xact_actions(true); /* close large objects before lower-level cleanup */ diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c index a07400e5ea7..59d9d06d5dd 100644 --- a/src/backend/executor/execParallel.c +++ b/src/backend/executor/execParallel.c @@ -1526,8 +1526,7 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc) } /* - * Estimate the amount of space required to serialize a - * session variable. + * Estimate the amount of space required to serialize a session variable. */ static Size EstimateSessionVariables(EState *estate) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 618325e4f56..806380bb8cd 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -4800,7 +4800,6 @@ substitute_actual_parameters_mutator(Node *node, return list_nth(context->args, param->paramid - 1); } } - return expression_tree_mutator(node, substitute_actual_parameters_mutator, (void *) context); } diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 702d875d0dd..d59fca31d20 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1400,6 +1400,7 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt) qry->hasWindowFuncs = pstate->p_hasWindowFuncs; qry->hasTargetSRFs = pstate->p_hasTargetSRFs; qry->hasAggs = pstate->p_hasAggs; + qry->hasSessionVariables = pstate->p_hasSessionVariables; foreach(l, stmt->lockingClause) { @@ -1407,8 +1408,6 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt) (LockingClause *) lfirst(l), false); } - qry->hasSessionVariables = pstate->p_hasSessionVariables; - assign_query_collations(pstate, qry); /* this must be done after collations, for reliable comparison of exprs */ @@ -1877,6 +1876,7 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt) qry->hasWindowFuncs = pstate->p_hasWindowFuncs; qry->hasTargetSRFs = pstate->p_hasTargetSRFs; qry->hasAggs = pstate->p_hasAggs; + qry->hasSessionVariables = pstate->p_hasSessionVariables; foreach(l, lockingClause) { @@ -1884,8 +1884,6 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt) (LockingClause *) lfirst(l), false); } - qry->hasSessionVariables = pstate->p_hasSessionVariables; - assign_query_collations(pstate, qry); /* this must be done after collations, for reliable comparison of exprs */ diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c index ab58a4d9df3..3e701723efb 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -1867,6 +1867,9 @@ ScanQueryForLocks(Query *parsetree, bool acquire) /* * Recurse into sublink subqueries, too. But we already did the ones in * the rtable and cteList. + * + * review: why are we checking hasSessionVariables too? Doesn't seem very + * clear, maybe explain in the comment. */ if (parsetree->hasSubLinks || parsetree->hasSessionVariables) diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 9f5f37a06bf..348c5f39407 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1480,7 +1480,7 @@ struct config_bool ConfigureNamesBool[] = }, { {"session_variables_ambiguity_warning", PGC_USERSET, DEVELOPER_OPTIONS, - gettext_noop("Raise warning when reference to session variable is ambiguous."), + gettext_noop("Raise warning when reference to a session variable is ambiguous."), NULL, GUC_NOT_IN_SAMPLE }, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 4bc2e9f2b37..a1d37d4512e 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11831,6 +11831,8 @@ prorettype => 'bytea', proargtypes => 'pg_brin_minmax_multi_summary', prosrc => 'brin_minmax_multi_summary_send' }, +# review: why "debug"? maybe have a list of session vars with a flag if they've +# been used, similar to pg_settings { oid => '8488', descr => 'debug list of used session variables', proname => 'pg_debug_show_used_session_variables', prorows => '1000', proretset => 't', provolatile => 's', prorettype => 'record', proargtypes => '', diff --git a/src/include/commands/session_variable.h b/src/include/commands/session_variable.h index aae3d251126..a0c99b9755b 100644 --- a/src/include/commands/session_variable.h +++ b/src/include/commands/session_variable.h @@ -26,6 +26,7 @@ extern void ResetSessionVariables(void); extern void SessionVariableCreatePostprocess(Oid varid, char eoxaction); extern void SessionVariableDropPostprocess(Oid varid); +/* review: seems duplicate with earlier prototype */ extern void SessionVariableCreatePostprocess(Oid varid, char eoxaction); extern Datum CopySessionVariable(Oid varid, bool *isNull, Oid *typid); diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 8939797e939..c646d84f256 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -158,7 +158,8 @@ typedef struct PlannerGlobal /* partition descriptors */ PartitionDirectory partition_directory pg_node_attr(read_write_ignore); - List *sessionVariables; /* list of used session variables */ + /* list of used session variables */ + List *sessionVariables; } PlannerGlobal; /* macro for fetching the Plan associated with a SubPlan node */ @@ -460,9 +461,9 @@ struct PlannerInfo bool placeholdersFrozen; /* true if planning a recursive WITH item */ bool hasRecursion; - /* true if session variables were used */ bool hasSessionVariables; + /* * Information about aggregates. Filled by preprocess_aggrefs(). */ diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 020a46dc2e0..92e018a2139 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -92,7 +92,7 @@ typedef struct PlannedStmt Node *utilityStmt; /* non-null if this is utility stmt */ - List *sessionVariables; /* list of OIDs for PARAM_VARIABLE Params */ + List *sessionVariables; /* OIDs for PARAM_VARIABLE Params */ /* statement location in source string (copied from Query) */ int stmt_location; /* start location, or -1 if unknown */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 238499d114c..877e459df99 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -44,8 +44,7 @@ typedef struct Alias } Alias; /* - * What to do at commit time for temporary relations or - * persistent/temporary variable. + * What to do at commit time for temporary relations or session variables. */ typedef enum OnCommitAction { -- 2.38.1