0004-review-comments-20221105.patch

From eaec2f9018e5e486f92138f6096bf32276bd9a87 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
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