From 141b31776fe38829ce5b77669c5d99148576dbbd Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 19 Mar 2022 16:04:51 -0500
Subject: [PATCH 3/4] [PATCH] Suggestion for comment improvements.

From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Tue, 1 Mar 2022 21:31:22 +0800
---
 src/backend/commands/sessionvariable.c   | 123 +++++++++++++----------
 src/backend/executor/execExpr.c          |  12 +--
 src/backend/executor/svariableReceiver.c |  11 +-
 src/backend/optimizer/plan/setrefs.c     |   5 +-
 src/backend/optimizer/util/clauses.c     |  27 +++--
 src/backend/parser/analyze.c             |  11 +-
 src/backend/parser/gram.y                |  10 +-
 src/backend/parser/parse_expr.c          |  27 ++---
 src/backend/utils/cache/lsyscache.c      |   2 +-
 src/include/catalog/pg_variable.h        |   2 +-
 10 files changed, 122 insertions(+), 108 deletions(-)

diff --git a/src/backend/commands/sessionvariable.c b/src/backend/commands/sessionvariable.c
index 7f130ced9d3..e07ca597bd1 100644
--- a/src/backend/commands/sessionvariable.c
+++ b/src/backend/commands/sessionvariable.c
@@ -72,12 +72,13 @@
  * the action list.
  *
  * We want to support the possibility of resetting a session
- * variable at the end of transaction. This ensures the initial
- * state of session variables at the begin of each transaction.
- * The reset is implemented as a removal of the session variable
- * from sessionvars hash table.  This enforce full initialization
- * in the next usage.  Careful though, this is not same as
- * dropping the session variable.
+ * variable at the end of transaction, with the ON TRANSACTION END
+ * RESET option. This ensures the initial state of session
+ * variables at the begin of each transaction.  The reset is
+ * implemented as a removal of the session variable from
+ * sessionvars hash table.  This enforce full initialization in
+ * the next usage.  Careful though, this is not same as dropping
+ * the session variable.
  *
  * Another functionality is dropping temporary session variable
  * with the option ON COMMIT DROP.
@@ -96,16 +97,15 @@ typedef struct SVariableXActActionItem
 	SVariableXActAction action;	/* reset or drop */
 
 	/*
-	 * If this entry was created during the current transaction,
-	 * creating_subid is the ID of the creating subxact. If deleted during
-	 * the current transaction, deleting_subid is the ID of the deleting
-	 * subxact. if no deletion request is pending, deleting_subid is zero.
+	 * creating_subid is the ID of the creating subxact. If the action was
+	 * unregistered during the current transaction, deleting_subid is the ID of
+	 * the deleting subxact, otherwise InvalidSubTransactionId.
 	 */
 	SubTransactionId creating_subid;
 	SubTransactionId deleting_subid;
 }  SVariableXActActionItem;
 
-/* Both lists holds field of SVariableXActActionItem type */
+/* Both lists hold fields of SVariableXActActionItem type */
 static List *xact_drop_actions = NIL;
 static List *xact_reset_actions = NIL;
 
@@ -122,7 +122,7 @@ typedef struct SVariableData
 	bool		is_rowtype;		/* true when variable is composite */
 	bool		is_not_null;	/* don't allow null values */
 	bool		is_immutable;	/* true when variable is immutable */
-	bool		has_defexpr;	/* true when there are default value */
+	bool		has_defexpr;	/* true when variable has a default value */
 
 	bool		is_valid;		/* true when variable was successfuly
 								 * initialized */
@@ -141,8 +141,8 @@ static void register_session_variable_xact_action(Oid varid, SVariableXActAction
 static void unregister_session_variable_xact_action(Oid varid, SVariableXActAction action);
 
 /*
- * Releases stored data from session variable. The hash entry
- * stay without change.
+ * Releases stored data from session variable, but preserve the hash entry
+ * in sessionvars.
  */
 static void
 free_session_variable_value(SVariable svar)
@@ -200,9 +200,11 @@ free_session_variable_varid(Oid varid)
 }
 
 /*
- * Assign sinval mark to session variable. This mark probably
- * signalized, so the session variable was dropped. But this
- * should be rechecked later against system catalog.
+ * Callback function for session variable invalidation.
+ *
+ * Register SVAR_RECHECK actions on appropriate currently cached
+ * session variables. Those will be processed later, rechecking against the
+ * catalog to detect dropped session variable.
  */
 static void
 pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
@@ -345,7 +347,7 @@ set_session_variable(SVariable svar, Datum value,
 
 /*
  * Initialize svar from var
- * svar - SVariable - holds data
+ * svar - SVariable - holds value
  * var  - Variable - holds metadata
  */
 static void
@@ -381,11 +383,13 @@ init_session_variable(SVariable svar, Variable *var)
 }
 
 /*
- * Try to search value in hash table. If it doesn't
+ * Search the given session variable in the hash table. If it doesn't
  * exist, then insert it (and calculate defexpr if it exists).
  *
+ * Caller is responsible for doing permission checks.
+ *
  * As side efect this function acquires AccessShareLock on
- * related session variable until commit.
+ * related session variable until the end of the transaction.
  */
 static SVariable
 prepare_variable_for_reading(Oid varid)
@@ -399,7 +403,7 @@ prepare_variable_for_reading(Oid varid)
 	if (!sessionvars)
 		create_sessionvars_hashtable();
 
-	/* Protect used session variable against drop until commit */
+	/* Protect used session variable against drop until transaction end */
 	LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);
 
 	svar = (SVariable) hash_search(sessionvars, &varid,
@@ -459,11 +463,13 @@ prepare_variable_for_reading(Oid varid)
 }
 
 /*
- * Write value to variable. We expect secured access in this moment.
+ * Store the given value in an SVariable, and cache it if not already present.
+ *
+ * Caller is responsible for doing permission checks.
  * We try not to break the previous value, if something is wrong.
  *
  * As side efect this function acquires AccessShareLock on
- * related session variable until commit.
+ * related session variable until the end of the transaction.
  */
 void
 SetSessionVariable(Oid varid, Datum value, bool isNull, Oid typid)
@@ -471,7 +477,7 @@ SetSessionVariable(Oid varid, Datum value, bool isNull, Oid typid)
 	SVariable svar;
 	bool		found;
 
-	/* Protect used session variable against drop until commit */
+	/* Protect used session variable against drop until transaction end */
 	LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);
 
 	if (!sessionvars)
@@ -485,7 +491,7 @@ SetSessionVariable(Oid varid, Datum value, bool isNull, Oid typid)
 	{
 		Variable	var;
 
-		/* don't need defexpr and acl here */
+		/* We don't need defexpr here */
 		initVariable(&var, varid, true);
 		init_session_variable(svar, &var);
 	}
@@ -494,8 +500,7 @@ SetSessionVariable(Oid varid, Datum value, bool isNull, Oid typid)
 }
 
 /*
- * Write value to variable with security check.
- * We try not to break the previous value, if something is wrong.
+ * Wrapper around SetSessionVariable after checking for correct permission.
  */
 void
 SetSessionVariableWithSecurityCheck(Oid varid, Datum value, bool isNull, Oid typid)
@@ -514,6 +519,7 @@ SetSessionVariableWithSecurityCheck(Oid varid, Datum value, bool isNull, Oid typ
 
 /*
  * Returns a copy of value of the session variable specified by varid
+ * Caller is responsible for doing permission checks.
  */
 Datum
 CopySessionVariable(Oid varid, bool *isNull, Oid *typid)
@@ -535,6 +541,7 @@ CopySessionVariable(Oid varid, bool *isNull, Oid *typid)
 /*
  * Returns the value of the session variable specified by varid. Check correct
  * result type. Optionally the result can be copied.
+ * Caller is responsible for doing permission checks.
  */
 Datum
 GetSessionVariable(Oid varid, bool *isNull, Oid expected_typid, bool copy)
@@ -695,28 +702,28 @@ RemoveSessionVariable(Oid varid)
 	table_close(rel, RowExclusiveLock);
 
 	/*
-	 * we removed entry from sys catalog already, we should not to do
-	 * again at xact time,
+	 * We removed entry from catalog already, we should not do it
+	 * again at end of xact time.
 	 */
 	unregister_session_variable_xact_action(varid, SVAR_ON_COMMIT_DROP);
 
 	/*
-	 * and if this transaction or subtransaction will be committed,
-	 * we want to enforce variable cleaning. (we don't need to wait for
+	 * And we want to enforce variable clearning when this transaction or
+	 * subtransaction will be committed (we don't need to wait for
 	 * sinval message). The cleaning action for one session variable
-	 * can be repeated in the action list, and it doesn't do any problem
-	 * (so we don't need to ensure uniqueness). We need separate action
-	 * than RESET, because RESET is executed on any transaction end,
-	 * but we want to execute cleaning only when thecurrent transaction
+	 * can be repeated in the action list without causing any problem,
+	 * so we don't need to ensure uniqueness. We need a different action
+	 * from RESET, because RESET is executed on any transaction end,
+	 * but we want to execute cleaning only when the current transaction
 	 * will be committed.
 	 */
 	register_session_variable_xact_action(varid, SVAR_ON_COMMIT_RESET);
 }
 
 /*
- * Fast drop complete content of all session variables hash table.
+ * Fast drop of the complete content of all session variables hash table.
  * This is code for DISCARD VARIABLES command. This command
- * cannot to run inside transaction, so we don't need to handle
+ * cannot be run inside transaction, so we don't need to handle
  * end of transaction actions.
  */
 void
@@ -734,7 +741,7 @@ ResetSessionVariables(void)
 		MemoryContextReset(SVariableMemoryContext);
 
 	/*
-	 * There are not any session variables, so trim
+	 * There are not any session variables left, so simply trim
 	 * both xact action lists.
 	 */
 	list_free_deep(xact_drop_actions);
@@ -824,11 +831,11 @@ ExecuteLetStmt(ParseState *pstate,
 }
 
 /*
- * Implementation of drop or reset actions executed on session variables
- * at transaction end time. We want to drop temporary session variables
- * with clause ON COMMIT DROP, or we want to reset values of session variables
- * with clause ON TRANSACTION END RESET or we want to clean (reset) memory
- * allocated by values of dropped session variables.
+ * Registration of actions to be executed on session variables at transaction
+ * end time. We want to drop temporary session variables with clause ON COMMIT
+ * DROP, or we want to reset values of session variables with clause ON
+ * TRANSACTION END RESET or we want to clean (reset) local memory allocated by
+ * values of dropped session variables.
  */
 
 /*
@@ -861,9 +868,10 @@ register_session_variable_xact_action(Oid varid,
 }
 
 /*
- * Remove variable from action list. In this moment,
- * the action is just marked as deleted by setting
- * deleting_subid.
+ * Unregister an action on a given session variable from action list. In this
+ * moment, the action is just marked as deleted by setting deleting_subid. The
+ * calling even might be rollbacked, in which case we should not lose this
+ * action.
  */
 static void
 unregister_session_variable_xact_action(Oid varid,
@@ -897,7 +905,7 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
 		SVariableXActActionItem *xact_ai =
 							(SVariableXActActionItem *) lfirst(l);
 
-		/* Iterate only over non dropped entries */
+		/* Iterate only over entries that are still pending */
 		if (xact_ai->deleting_subid == InvalidSubTransactionId)
 		{
 			Assert(xact_ai->action == SVAR_ON_COMMIT_DROP);
@@ -928,6 +936,10 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
 		}
 	}
 
+	/*
+	 * Any drop action left is an entry that was unregistered and not
+	 * rollbacked, so we can simply remove them.
+	 */
 	list_free_deep(xact_drop_actions);
 	xact_drop_actions = NIL;
 
@@ -939,10 +951,11 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
 		if (xact_ai->action == SVAR_RECHECK)
 		{
 			/*
-			 * we can do recheck only when transactionn is commited
-			 * and we can safely touch system catalogue. When transaction
-			 * is ROLLBACKed, then we should to postpone check to next
-			 * transaction.
+			 * We can do the recheck only when the transaction is commited as
+			 * we need to access system catalog. When transaction is
+			 * ROLLBACKed, then we have to postpone the check to next
+			 * transaction. We therefore don't check for a matching
+			 * creating_subid here.
 			 */
 			if (isCommit)
 			{
@@ -983,6 +996,10 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
 				 isCommit))
 				free_session_variable_varid(xact_ai->varid);
 
+			/*
+			 * Any non SVAR_RECHECK action can now be removed.  It was either
+			 * explicitly processed, or implicitly due to some ROLLBACK action.
+			 */
 			xact_reset_actions = foreach_delete_current(xact_reset_actions, l);
 			pfree(xact_ai);
 		}
@@ -993,7 +1010,7 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
  * Post-subcommit or post-subabort cleanup of xact action list.
  *
  * During subabort, we can immediately remove entries created during this
- * subtransaction. During subcommit, just relabel entries marked during
+ * subtransaction. During subcommit, just transfer entries marked during
  * this subtransaction as being the parent's responsibility.
  */
 void
@@ -1024,7 +1041,7 @@ AtEOSubXact_SessionVariable_on_xact_actions(bool isCommit, SubTransactionId mySu
 	}
 
 	/*
-	 * Reset and recheck actions - cleaning memory should be used every time
+	 * Reset and recheck actions - cleaning memory should be done every time
 	 * (when the variable with short life cycle was used) and then
 	 * cannot be removed from xact action list.
 	 */
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 2fb068d993d..80c064e9a81 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1007,27 +1007,23 @@ ExecInitExprRec(Expr *node, ExprState *state,
 								es_num_session_variables = state->parent->state->es_num_session_variables;
 							}
 
-							/*
-							 * We should use session variable buffer, when
-							 * it is available.
-							 */
+							/* Use session variable buffer when available. */
 							if (es_session_variables)
 							{
 								SessionVariableValue *var;
 
-								/* check params, unexpected */
+								/* Parameter sanity checks. */
 								if (param->paramid >= es_num_session_variables)
 									elog(ERROR, "paramid of PARAM_VARIABLE param is out of range");
 
 								var = &es_session_variables[param->paramid];
 
-								/* unexpected */
 								if (var->typid != param->paramtype)
 									elog(ERROR, "type of buffered value is different than PARAM_VARIABLE type");
 
 								/*
 								 * In this case, the parameter is like a
-								 * constant
+								 * constant.
 								 */
 								scratch.opcode = EEOP_CONST;
 								scratch.d.constval.value = var->value;
@@ -1037,7 +1033,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
 							else
 							{
 								/*
-								 * When we have no full PlanState (plpgsql
+								 * When we don't have a full PlanState (plpgsql
 								 * simple expr evaluation), then we should
 								 * use direct access.
 								 */
diff --git a/src/backend/executor/svariableReceiver.c b/src/backend/executor/svariableReceiver.c
index d5ce377b0fe..2cede4ebae3 100644
--- a/src/backend/executor/svariableReceiver.c
+++ b/src/backend/executor/svariableReceiver.c
@@ -99,7 +99,7 @@ svariableReceiveSlot(TupleTableSlot *slot, DestReceiver *self)
 static void
 svariableShutdownReceiver(DestReceiver *self)
 {
-	/* Do nothing */
+	/* Nothing to do. */
 }
 
 /*
@@ -125,13 +125,16 @@ CreateVariableDestReceiver(void)
 	self->pub.rDestroy = svariableDestroyReceiver;
 	self->pub.mydest = DestVariable;
 
-	/* private fields will be set by SetVariableDestReceiverParams */
-
+	/*
+	 * Private fields will be set by SetVariableDestReceiverParams and
+	 * svariableStartupReceiver.
+	 */
 	return (DestReceiver *) self;
 }
 
 /*
- * Set parameters for a VariableDestReceiver
+ * Set parameters for a VariableDestReceiver.
+ * Should be called right after creating the DestReceiver.
  */
 void
 SetVariableDestReceiverParams(DestReceiver *self, Oid varid)
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 7486d975c79..56edecad514 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1749,8 +1749,9 @@ copyVar(Var *var)
  * This is code that is common to all variants of expression-fixing.
  * We must look up operator opcode info for OpExpr and related nodes,
  * add OIDs from regclass Const nodes into root->glob->relationOids, and
- * add PlanInvalItems for user-defined functions into root->glob->invalItems.
- * We also fill in column index lists for GROUPING() expressions.
+ * add PlanInvalItems for user-defined functions and session variables into
+ * root->glob->invalItems.  We also fill in column index lists for GROUPING()
+ * expressions.
  *
  * We assume it's okay to update opcode info in-place.  So this could possibly
  * scribble on the planner's input data structures, but it's OK.
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b83665dedd5..f9d01903ad1 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -811,10 +811,10 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context)
 
 	/*
 	 * We can't pass Params to workers at the moment either, so they are also
-	 * parallel-restricted, unless they are PARAM_EXTERN Params or are
-	 * PARAM_EXEC Params listed in safe_param_ids, meaning they could be
-	 * either generated within workers or can be computed by the leader and
-	 * then their value can be passed to workers.
+	 * parallel-restricted, unless they are PARAM_EXTERN or PARAM_VARIABLE
+	 * Params or are PARAM_EXEC Params listed in safe_param_ids, meaning they
+	 * could be either generated within workers or can be computed by the
+	 * leader and then their value can be passed to workers.
 	 */
 	else if (IsA(node, Param))
 	{
@@ -4764,20 +4764,19 @@ substitute_actual_parameters_mutator(Node *node,
 		return NULL;
 
 	/*
-	 * The expression of SQL function can contain params of two separated
-	 * by different paramkind field. The nodes with paramkind PARAM_EXTERN
-	 * are related to function's arguments (and should be replaced in this
-	 * step), because this is mechanism how we apply the function's arguments
-	 * for an expression.
+	 * SQL functions can contain two different kind of params. The nodes with
+	 * paramkind PARAM_EXTERN are related to function's arguments (and should
+	 * be replaced in this step), because this is how we apply the function's
+	 * arguments for an expression.
 	 *
-	 * The nodes with paramkind PARAM_VARIABLE are related to an usage of
+	 * The nodes with paramkind PARAM_VARIABLE are related to usage of
 	 * session variables. The values of session variables are not passed
 	 * to expression by expression arguments, so it should not be replaced
-	 * here by function's arguments. Although we can substitute params
+	 * here by function's arguments. Although we could substitute params
 	 * related to immutable session variables with default expression by
-	 * this default expression, it is safer don't do it. We don't need to
-	 * run security checks here. There can be some performance loss, but
-	 * an access to session variable is fast (and the result of default
+	 * this default expression, it is safer to not do it. This way we don't
+	 * have to run security checks here. There can be some performance loss,
+	 * but an access to session variable is fast (and the result of default
 	 * expression is immediately materialized and can be reused).
 	 */
 	if (IsA(node, Param))
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index f08d75c2a19..43a11603932 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1727,10 +1727,8 @@ transformLetStmt(ParseState *pstate, LetStmt * stmt)
 	pstate->p_expr_kind = EXPR_KIND_LET_TARGET;
 
 	/*
-	 * stmt->query is SelectStmt node. An tranformation of
-	 * this node doesn't support SetToDefault node. Instead injecting
-	 * of transformSelectStmt or parse state, we can directly
-	 * transform target list here if holds SetToDefault node.
+	 * stmt->query is a SelectStmt node. We don't tranform this node to a
+	 * SetToDefault node. Instead  we directly transform the target list here.
 	 */
 	if (stmt->set_default)
 	{
@@ -1895,9 +1893,8 @@ transformLetStmt(ParseState *pstate, LetStmt * stmt)
 	stmt->query = (Node *) query;
 
 	/*
-	 * When statement is executed as an expression as PLpgSQL LET
-	 * statement, then we should return query. We should not
-	 * use a utility wrapper node.
+	 * When statement is executed as a PlpgSQL LET statement, then we should
+	 * return the query and not use a utilityStmt wrapper node.
 	 */
 	if (stmt->plpgsql_mode)
 		return query;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index bf103ab1ed8..a19c56fadbd 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4816,11 +4816,11 @@ OptSessionVarDefExpr: DEFAULT b_expr					{ $$ = $2; }
 
 /*
  * Temporary session variables can be dropped on successful
- * transaction end like tables. We can force only reset on
- * transaction end. Because the session variables are not
- * transactional, we have calculate with ROLLBACK too.
- * The clause ON TRANSACTION END is more illustrative
- * synonymum to ON COMMIT ROLLBACK RESET.
+ * transaction end like tables.  RESET can only be forced on
+ * transaction end. Since the session variables are not
+ * transactional, we have to handle ROLLBACK too.
+ * The clause ON TRANSACTION END is clearer than some
+ * ON COMMIT ROLLBACK RESET clause.
  */
 OnEOXActionOption:  ON COMMIT DROP					{ $$ = VARIABLE_EOX_DROP; }
 			| ON TRANSACTION END_P RESET			{ $$ = VARIABLE_EOX_RESET; }
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 060d74814b0..04dbb2bdcf4 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -797,10 +797,10 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 					 parser_errposition(pstate, cref->location)));
 	}
 
-	/* Protect session variable by AccessShareLock when it is not shadowed */
+	/* Protect session variable by AccessShareLock when it is not shadowed. */
 	lockit = (node == NULL);
 
-	/* The col's reference can be reference to session variable too. */
+	/* The col's reference can be a reference to session variable too. */
 	varid = IdentifyVariable(cref->fields, &attrname, lockit, &not_unique);
 
 	/*
@@ -823,12 +823,11 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 		if (node != NULL)
 		{
 			/*
-			 * In this case we can raise warning (when it is required).
-			 * These warnings can be reduced. We should not to raise
-			 * warning for contexts where usage of session variables
-			 * has not sense. We should not to raise warning when we
-			 * can detect so variable has not wanted field (and then
-			 * there is not possible identifier's collision).
+			 * In this case we can raise a warning, but only when required.
+			 * We should not raise warnings for contexts where usage of session
+			 * variables has not sense, or when we can detect that variable
+			 * doesn't have the wanted field (and then there is no possible
+			 * identifier's collision).
 			 */
 			if (session_variables_ambiguity_warning &&
 				pstate->p_expr_kind == EXPR_KIND_SELECT_TARGET)
@@ -843,15 +842,16 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 
 				/*
 				 * Some cases with ambiguous references can be solved without
-				 * raising an warning. When there is an collision between column
+				 * raising a warning. When there is a collision between column
 				 * name (or label) and some session variable name, and when we
 				 * know attribute name, then we can ignore the collision when:
 				 *
 				 *   a) variable is of scalar type (then indirection cannot be
 				 *      applied on this session variable.
 				 *
-				 *   b) when related variable has no field of attrname, then
-				 *      indirection cannot be applied on this session variable.
+				 *   b) when related variable has no field with the given
+				 *      attrname, then indirection cannot be applied on this
+				 *      session variable.
 				 */
 				if (attrname)
 				{
@@ -885,7 +885,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 				}
 
 				/*
-				 * Raise warning when session variable reference is still valid.
+				 * Raise warning when session variable reference is still
+				 * visible.
 				 */
 				if (OidIsValid(varid))
 					ereport(WARNING,
@@ -896,7 +897,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 							 parser_errposition(pstate, cref->location)));
 			}
 
-			/* Reference to session variable is shadowed by any other always. */
+			/* Session variables are always shadowed by any other node. */
 			varid = InvalidOid;
 		}
 		else
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index f12166ba95c..4459b182872 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3597,7 +3597,7 @@ get_varname_varid(const char *varname, Oid varnamespace)
 
 /*
  * get_session_variable_name
- *		Returns the name of a given session variable.
+ *		Returns a palloc'd copy of the name of a given session variable.
  */
 char *
 get_session_variable_name(Oid varid)
diff --git a/src/include/catalog/pg_variable.h b/src/include/catalog/pg_variable.h
index 2754b48dde9..3dd4455921d 100644
--- a/src/include/catalog/pg_variable.h
+++ b/src/include/catalog/pg_variable.h
@@ -55,7 +55,7 @@ typedef enum VariableEOXAction
 {
 	VARIABLE_EOX_NOOP = 'n',	/* NOOP */
 	VARIABLE_EOX_DROP = 'd',	/* ON COMMIT DROP */
-	VARIABLE_EOX_RESET = 'r',	/* ON COMMIT RESET */
+	VARIABLE_EOX_RESET = 'r',	/* ON TRANSACTION END RESET */
 }			VariableEOXAction;
 
 /* ----------------
-- 
2.17.1

