From 84ede1a9e61ba08fda7cfb2073714a79140af581 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Thu, 15 Sep 2022 23:33:42 +0800
Subject: [PATCH v20220922 04/13] FIXUP 0003-session-variables

---
 src/backend/commands/session_variable.c | 561 +++++++++++++-----------
 src/include/commands/session_variable.h |   4 +-
 2 files changed, 311 insertions(+), 254 deletions(-)

diff --git a/src/backend/commands/session_variable.c b/src/backend/commands/session_variable.c
index d34f193284..53b0fe58a0 100644
--- a/src/backend/commands/session_variable.c
+++ b/src/backend/commands/session_variable.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  src/backend/commands/sessionvariable.c
+ *	  src/backend/commands/session_variable.c
  *
  *-------------------------------------------------------------------------
  */
@@ -36,62 +36,57 @@
  * Values of session variables are stored in the backend local memory,
  * in sessionvars hash table in binary format, in a dedicated memory
  * context SVariableMemoryContext.  A session variable value can stay
- * valid for longer than a transaction.  To make sure that the
- * underlying memory is freed, we need to solve following issues:
+ * valid for longer than the transaction that assigns its value.  To
+ * make sure that the underlying memory is eventually freed, but not
+ * before it's guarantee that the value won't be needed anymore, we
+ * need to handle the two following points:
  *
- * - We need to free local memory when the variable is droppedn,
- *  whether in the current transaction in the current session or by
- *  other sessions (other users).  To protect the content against
- *  possibly rollbacked DROP VARIABLE commands, the memory shouldn't
- *  be freed immediately but be postponed until the end of the
- *  transaction.
+ * - We need detect when a variable is dropped, whether in the current
+ * transaction in the current session or by another session, and mark
+ * the underlying entries for removal.  To protect the content against
+ * possibly rollbacked DROP VARIABLE commands, the entries (and
+ * memory) shouldn't be freed immediately but be postponed until the
+ * end of the transaction.
  *
  * - The session variable can be dropped explicitly (by DROP VARIABLE
  *  command) or implicitly (using ON COMMIT DROP clause), and the
  *  value can be implicitly removed (using the ON TRANSACTION END
- *  clause).  In all those cases the memory should be freed at the
- *  transaction end.
+ *  clause).  In all those cases the memory should also be freed at
+ *  the transaction end.
  *
- * To achieve that, we maintain 4 queues (implemented as list
- * of variable oid, list of RecheckVariableRequests
- * (requests are created in reaction on sinval) and lists
- * of actions). List of actions are used whe we need to calculate
- * the final transaction state of an entry's creator's
- * subtransaction. List of session variables are used, when we
- * don't need to do this calculation (we know, so. every values
- * of session variables created with clauses ON COMMIT DROP or
- * ON TRANSACTION END RESET will be purged from memory), but
- * ON COMMIT DROP action or ON COMMIT RESET action can be done only
- * when related sub transaction is committed (we don't want to lost
- * value after aborted DROP VARIABLE command, we don't want to delete
- * record from system catalog after aborted CREATE VARIABLE command,
- * or we don't want to drop record from system catalog implicitly
- * when TEMP ON COMMIT DROP variable was successfully dropped).
- * Recheck of session's variable existence should be executed
- * only when some operation over session's variable is executed
- * first time in transaction or at the end of transaction. It
- * should not be executed more times inside transaction.
+ * To achieve that, we maintain 3 queues of actions to be performed at
+ * certain time:
+ * - a List of SVariableXActActionItem, to handle ON COMMIT DROP
+ * variables, and delayed memory cleanup of variable dropped by the
+ * current transaction.  Those actions are transactional (for instance
+ * we don't want to cleanup the memory of a rollbacked DROP VARIABLE)
+ * so the structure is needed to keep track of the final transaction
+ * state
+ * - a List of variable Oid for the ON TRANSACTION ON RESET variables
+ * - a List of variable Oid for the concurrent DROP VARIABLE
+ * notification we receive via shared invalidations.
  *
- * Although the variable's reset doesn't need full delete from
- * sessionvars hash table, we do. It is the most simple solution,
- * and it reduces possible state's space (and reduces sessionvars
- * bloating).
+ * Note that although resetting a variable doesn't technically require
+ * to remove the entry from the sessionvars hash table, we currently
+ * do it.  It's a simple way to implement the reset, and helps to
+ * reduce memory usage and prevents the hash table from bloating.
  *
- * There are two different ways to do final access to session
+ * There are two different ways to do the final access to session
  * variables: buffered (indirect) or direct. Buffered access is used
- * in queries, where we have to ensure an stability of passed values
- * (then the session variable has same behaviour like external query
- * parameters, what is consistent with using PL/pgSQL's variables in
- * embedded queries in PL/pgSQL).
+ * in regular DML statements, where we have to ensure the stability of
+ * the variable values.  The session variables have the same behaviour
+ * as external query parameters, which is consistent with using
+ * PL/pgSQL's variables in embedded queries in PL/pgSQL.
  *
  * This is implemented by using an aux buffer (an array) that holds a
- * copy of values of used (in query) session variables. In the final
- * end, the values from this array are passed as constant (EEOP_CONST).
+ * copy of values of used (in query) session variables, which is also
+ * transmitted to the parallel workers.  The values from this array
+ * are passed as constant (EEOP_CONST).
  *
  * Direct access is used by simple expression evaluation (PLpgSQL).
  * In this case we don't need to ensure the stability of passed
  * values, and maintaining the buffer with copies of values of session
- * variables can be useless overhead. In this case we just read the
+ * variables would be useless overhead.  In this case we just read the
  * value of the session variable directly (EEOP_PARAM_VARIABLE). This
  * strategy removes the necessity to modify related PL/pgSQL code to
  * support session variables (the reading of session variables is
@@ -109,20 +104,21 @@ typedef struct SVariableXActActionItem
 	SVariableXActAction action;
 
 	/*
-	 * 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.
+	 * creating_subid is the ID of the sub-transaction that registered
+	 * the action. If the action was unregistered during the current
+	 * transaction, deleting_subid is the ID of the deleting
+	 * sub-transaction, otherwise InvalidSubTransactionId.
 	 */
 	SubTransactionId creating_subid;
 	SubTransactionId deleting_subid;
 } SVariableXActActionItem;
 
-/* list holds fields of SVariableXActActionItem type */
+/* List of SVariableXActActionItem */
 static List *xact_on_commit_actions = NIL;
 
 /*
- * the ON COMMIT DROP and ON TRANSACTION END RESET variables
- * are purged from memory every time.
+ * To process ON TRANSACTION END RESET variables, for which we always
+ * need to clear the saved values.
  */
 static List *xact_reset_varids = NIL;
 
@@ -134,25 +130,28 @@ static List *xact_reset_varids = NIL;
  * hold list of possibly dropped session variables and at the end of
  * transaction, we check session variables from this list against system
  * catalog. This check can be postponed into next transaction if
- * current transactions is in aborted state.
+ * current transactions is in aborted state, as we wouldn't be able to
+ * access the system catalog.
  */
 static List *xact_recheck_varids = NIL;
 
 typedef struct SVariableData
 {
-	Oid			varid;			/* pg_variable OID of this sequence (hash key) */
+	Oid			varid;			/* pg_variable OID of this sequence
+								   (hash key) */
 
 	/*
 	 * The session variable is identified by oid. The oid is unique in
-	 * catalog. Unfortunately, the cleaning memory can be postponed to begin
-	 * of next transaction in session, and theoreticaly, there can be
-	 * different session variable with same oid. So we need another extra
-	 * identifier that helps with identity check. We can use LSN number in
-	 * session variable create time. The value of session variable (in memory)
-	 * is valid, when there is an record in pg_variable with same oid and same
-	 * create_lsn.
+	 * catalog. Unfortunately, the memory cleanup can be postponed to
+	 * the beginning
+	 * of the session next transaction, and it's possible that this next
+	 * transaction sees a different variable with the same oid.  We
+	 * therefore need an extra identifier to distinguish both cases. We
+	 * use the LSN number of session variable at creation time. The
+	 * value of session variable (in memory) is valid, when there is a
+	 * record in pg_variable with same oid and same create_lsn.
 	 */
-	XLogRecPtr	create_lsn;		/* used for session's variable identity check */
+	XLogRecPtr	create_lsn;
 
 	bool		isnull;
 	bool		freeval;
@@ -169,7 +168,7 @@ typedef struct SVariableData
 	/*
 	 * Top level local transaction id of the last transaction that dropped the
 	 * variable if any.  We need this information to avoid freeing memory for
-	 * variable dropped by the local backend that may be rollbacked.
+	 * variable dropped by the local backend that may be eventually rollbacked.
 	 */
 	LocalTransactionId drop_lxid;
 
@@ -189,22 +188,33 @@ typedef struct SVariableData
 typedef SVariableData *SVariable;
 
 static HTAB *sessionvars = NULL;	/* hash table for session variables */
-static HTAB *sessionvars_types = NULL;	/* hash table for type fingerprints of
-										 * session variables */
 
 static MemoryContext SVariableMemoryContext = NULL;
 
+static void create_sessionvars_hashtables(void);
+static void free_session_variable_value(SVariable svar);
 static void init_session_variable(SVariable svar, Variable *var);
-
-static void register_session_variable_xact_action(Oid varid, SVariableXActAction action);
-static void unregister_session_variable_xact_action(Oid varid, SVariableXActAction action);
+static bool is_session_variable_valid(SVariable svar);
+static void pg_variable_cache_callback(Datum arg, int cacheid,
+									   uint32 hashvalue);
+static SVariable prepare_variable_for_reading(Oid varid);
+static void register_session_variable_xact_action(Oid varid,
+												  SVariableXActAction action);
+static void remove_session_variable(SVariable svar);
+static void remove_session_variable_by_id(Oid varid);
+static void set_session_variable(SVariable svar, Datum value, bool isnull,
+								 bool init_mode);
+static const char *SVariableXActActionName(SVariableXActAction action);
+static void sync_sessionvars_all(void);
+static void unregister_session_variable_xact_action(Oid varid,
+													SVariableXActAction action);
 
 
 /*
  * Returns human readable name of SVariableXActAction value.
  */
 static const char *
-SvariableXActActionName(SVariableXActAction action)
+SVariableXActActionName(SVariableXActAction action)
 {
 	switch (action)
 	{
@@ -213,15 +223,14 @@ SvariableXActActionName(SVariableXActAction action)
 		case SVAR_ON_COMMIT_RESET:
 			return "ON COMMIT RESET";
 		default:
-			elog(ERROR, "unknown SVariableXActAction action %d",
-				 action);
+			elog(ERROR, "unknown SVariableXActAction action %d", action);
 	}
 }
 
 /*
- * Releases stored data from session variable, but preserve the hash entry
- * in sessionvars. When we replace row value by new value with same type
- * fingerprint, we can keep field desc data.
+ * Free all memory allocated for the given session variable, but
+ * preserve the hash entry in sessionvars.
+ * This function shouldn't contain anything that could ereport(ERROR).
  */
 static void
 free_session_variable_value(SVariable svar)
@@ -245,8 +254,65 @@ free_session_variable_value(SVariable svar)
 }
 
 /*
- * Release the variable defined by varid from sessionvars
- * hashtab.
+ * 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 clean (reset) local memory allocated by
+ * values of session variables dropped in other backends.
+ */
+static void
+register_session_variable_xact_action(Oid varid,
+									  SVariableXActAction action)
+{
+	SVariableXActActionItem *xact_ai;
+	MemoryContext oldcxt;
+
+	elog(DEBUG1, "SVariableXActAction \"%s\" is registered for session variable (oid:%u)",
+		 SVariableXActActionName(action), varid);
+
+	oldcxt = MemoryContextSwitchTo(TopTransactionContext);
+
+	xact_ai = (SVariableXActActionItem *)
+		palloc(sizeof(SVariableXActActionItem));
+
+	xact_ai->varid = varid;
+	xact_ai->action = action;
+
+	xact_ai->creating_subid = GetCurrentSubTransactionId();
+	xact_ai->deleting_subid = InvalidSubTransactionId;
+
+	xact_on_commit_actions = lcons(xact_ai, xact_on_commit_actions);
+
+	MemoryContextSwitchTo(oldcxt);
+}
+
+/*
+ * Unregister an action on a given session variable from the action list.
+ * The action is just marked as deleted by setting deleting_subid.
+ * The calling subtransaction even might be rollbacked, in which case the
+ * action shouldn't be removed.
+ */
+static void
+unregister_session_variable_xact_action(Oid varid,
+										SVariableXActAction action)
+{
+	ListCell   *l;
+
+	elog(DEBUG1, "SVariableXActAction \"%s\" is unregistered for session variable (oid:%u)",
+		 SVariableXActActionName(action), varid);
+
+	foreach(l, xact_on_commit_actions)
+	{
+		SVariableXActActionItem *xact_ai =
+		(SVariableXActActionItem *) lfirst(l);
+
+		if (xact_ai->action == action && xact_ai->varid == varid)
+			xact_ai->deleting_subid = GetCurrentSubTransactionId();
+	}
+}
+
+/*
+ * Release the given session variable from sessionvars hashtab and free
+ * all underlying allocated memory.
  */
 static void
 remove_session_variable(SVariable svar)
@@ -268,8 +334,8 @@ remove_session_variable(SVariable svar)
 }
 
 /*
- * Release the variable defined by varid from sessionvars
- * hashtab.
+ * Release the session variable defined by varid from sessionvars
+ * hashtab and free all underlying allocated memory.
  */
 static void
 remove_session_variable_by_id(Oid varid)
@@ -288,6 +354,8 @@ remove_session_variable_by_id(Oid varid)
 
 /*
  * Callback function for session variable invalidation.
+ *
+ * It queues a list of variable Oid in xact_recheck_varids.
  */
 static void
 pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
@@ -308,8 +376,8 @@ pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
 	/*
 	 * When the hashvalue is not specified, then we have to recheck all
 	 * currently used session variables. Since we can't guarantee the exact
-	 * session variable from its hashValue, we have to iterate over all items
-	 * of hash table.
+	 * session variable from its hashValue, we also have to iterate over
+	 * all items of the sessionvars hash table.
 	 */
 	hash_seq_init(&status, sessionvars);
 
@@ -332,14 +400,15 @@ pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
 
 		/*
 		 * although it there is low probability, we have to iterate over all
-		 * locally set session variables, because hashvalue is not unique
+		 * locally set session variables, because hashvalue is not a unique
 		 * identifier.
 		 */
 	}
 }
 
 /*
- * Returns true, when the entry in pg_variable is valid for SVariable
+ * Returns true when the entry in pg_variable is valid for the given session
+ * variable.
  */
 static bool
 is_session_variable_valid(SVariable svar)
@@ -353,8 +422,9 @@ is_session_variable_valid(SVariable svar)
 	{
 		/*
 		 * In this case, the only oid cannot be used as unique identifier,
-		 * because after oid's reset the oid can be used for new other session
-		 * variable. We do second check against 64bit unique identifier.
+		 * because the oid counter can wraparound, and the oid can be used for
+		 * new other session variable. We do a second check against 64bit
+		 * unique identifier.
 		 */
 		if (svar->create_lsn == ((Form_pg_variable) GETSTRUCT(tp))->create_lsn)
 			result = true;
@@ -366,12 +436,12 @@ is_session_variable_valid(SVariable svar)
 }
 
 /*
- * The possible invalidated variables (in memory) are checked
- * against system catalog. This routine is called before
- * any read or any write from/to session variable.
+ * Recheck the possibly invalidated variables (in memory) against system
+ * catalog. This routine is called before any read or any write from/to session
+ * variables.
  */
 static void
-sync_sessionvars_all()
+sync_sessionvars_all(void)
 {
 	SVariable	svar;
 	ListCell   *l;
@@ -380,8 +450,9 @@ sync_sessionvars_all()
 		return;
 
 	/*
-	 * sessionvars is null after DISCARD VARIABLES. When we are sure, so there
-	 * are not any active session variable in this session.
+	 * If the sessionvars hashtable is NULL (which can be done by DISCARD
+	 * VARIABLES), we are sure that there aren't any active session variable
+	 * in this session.
 	 */
 	if (!sessionvars)
 	{
@@ -394,7 +465,7 @@ sync_sessionvars_all()
 
 	/*
 	 * This routine is called before any reading, so the session should be in
-	 * transaction state. This is required for access to system catalog.
+	 * transaction state. This is required to access the system catalog.
 	 */
 	Assert(IsTransactionState());
 
@@ -408,9 +479,10 @@ sync_sessionvars_all()
 
 		/*
 		 * Remove invalid variables, but don't touch variables that were
-		 * dropped by the current top level local transaction, as there's no
-		 * guarantee that the transaction will be committed.  Such variables
-		 * will be removed in the next transaction if needed.
+		 * dropped by the current top level local transaction or any
+		 * subtransaction underneath, as there's no guarantee that the
+		 * transaction will be committed.  Such variables will be removed in
+		 * the next transaction if needed.
 		 */
 		if (found)
 		{
@@ -430,7 +502,7 @@ sync_sessionvars_all()
 }
 
 /*
- * Create the hash table for storing session variables
+ * Create the hash table for storing session variables.
  */
 static void
 create_sessionvars_hashtables(void)
@@ -471,12 +543,11 @@ create_sessionvars_hashtables(void)
  * Assign some content to the session variable. It's copied to
  * SVariableMemoryContext if necessary.
  *
- * init_mode is true, when the value of session variable is being initialized
- * by default expression or by null. Only in this moment we can allow to
- * modify immutable variables with default expression.
+ * init_mode is true when the value of session variable should be initialized
+ * by the default expression if any. This is the only case where we allow the
+ * modification of an immutable variables with default expression.
  *
- * This routine have to do successfull change or leave memory without
- * change.
+ * If any error happens, the existing value shouldn't be modified.
  */
 static void
 set_session_variable(SVariable svar, Datum value, bool isnull, bool init_mode)
@@ -494,10 +565,10 @@ set_session_variable(SVariable svar, Datum value, bool isnull, bool init_mode)
 						get_session_variable_name(svar->varid))));
 
 	/*
-	 * Don't allow updating of immutable session variable that has assigned
-	 * not null value or has default expression (and then the value should be
-	 * result of default expression always). Don't do this check, when
-	 * variable is being initialized.
+	 * Don't allow the modification of an immutable session variable that
+	 * already has an assigned value (possibly NULL) or has a default
+	 * expression (in which case the value should always be the result of
+	 * default expression evaluation) unless the variable is being initialized.
 	 */
 	if (!init_mode &&
 		(svar->is_immutable &&
@@ -516,6 +587,16 @@ set_session_variable(SVariable svar, Datum value, bool isnull, bool init_mode)
 
 		MemoryContextSwitchTo(oldcxt);
 	}
+	else
+	{
+		/* The caller shouldn't have provided any real value. */
+		Assert(value == (Datum) 0);
+	}
+
+	/*
+	 * No error should happen after this poiht, otherwise we could leak the
+	 * newly allocated value if any.
+	 */
 
 	free_session_variable_value(svar);
 
@@ -525,6 +606,13 @@ set_session_variable(SVariable svar, Datum value, bool isnull, bool init_mode)
 	svar->freeval = newval != value;
 	svar->is_valid = true;
 
+	/*
+	 * XXX While unlikely, an error here is possible.
+	 * It wouldn't leak memory as the allocated chunk has already been
+	 * correctly assigned to the session variable, but would contradict this
+	 * function contract, which is that this function should either succeed or
+	 * leave the current value untouched.
+	 */
 	elog(DEBUG1, "session variable \"%s.%s\" (oid:%u) has new value",
 		 get_namespace_name(get_session_variable_namespace(svar->varid)),
 		 get_session_variable_name(svar->varid),
@@ -532,9 +620,7 @@ set_session_variable(SVariable svar, Datum value, bool isnull, bool init_mode)
 }
 
 /*
- * Initialize svar from var
- * svar - SVariable - holds value
- * var  - Variable - holds metadata
+ * Initialize session variable svar from variable var
  */
 static void
 init_session_variable(SVariable svar, Variable *var)
@@ -580,8 +666,8 @@ init_session_variable(SVariable svar, Variable *var)
 }
 
 /*
- * Search the given session variable in the hash table. If it doesn't
- * exist, then insert it (and calculate defexpr if it exists).
+ * Search a seesion variable in the hash table given its oid. If it
+ * doesn't exist, then insert it (and calculate defexpr if it exists).
  *
  * Caller is responsible for doing permission checks.
  *
@@ -600,12 +686,12 @@ prepare_variable_for_reading(Oid varid)
 	if (!sessionvars)
 		create_sessionvars_hashtables();
 
-	/* Ensure so all entries in sessionvars hash table are valid */
-	sync_sessionvars_all();
-
 	/* Protect used session variable against drop until transaction end */
 	LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);
 
+	/* Make sure that all entries in sessionvars hash table are valid */
+	sync_sessionvars_all();
+
 	svar = (SVariable) hash_search(sessionvars, &varid,
 								   HASH_ENTER, &found);
 
@@ -625,7 +711,10 @@ prepare_variable_for_reading(Oid varid)
 				 varid);
 		}
 
-		/* Raise an error when we cannot initialize variable correctly */
+		/*
+		 * Raise an error if this is a NOT NULL variable without default
+		 * expression.
+		 */
 		if (var.is_not_null && !var.defexpr)
 			ereport(ERROR,
 					(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
@@ -668,16 +757,18 @@ prepare_variable_for_reading(Oid varid)
 	/*
 	 * Although the value of domain type should be valid (it is checked when
 	 * it is assigned to session variable), we have to check related
-	 * constraints anytime. It can be more expensive than in PL/pgSQL.
-	 * PL/pgSQL forces domain checks when value is assigned to the variable or
-	 * when value is returned from function. Fortunately, domain types manage
-	 * cache of constraints by self.
+	 * constraints each time we access the variable. It can be more expensive
+	 * than in PL/pgSQL, as PL/pgSQL forces domain checks only when the value is assigned
+	 * to the variable or when the value is returned from function.
+	 * However, domain types have a constraint cache so it's not too much
+	 * expensive..
 	 */
 	if (svar->is_domain)
 	{
 		/*
 		 * Store domain_check extra in TopTransactionContext. When we are in
-		 * other transaction, the domain_check_extra cache is not valid.
+		 * other transaction, the domain_check_extra cache is not valid
+		 * anymore.
 		 */
 		if (svar->domain_check_extra_lxid != MyProc->lxid)
 			svar->domain_check_extra = NULL;
@@ -696,7 +787,6 @@ prepare_variable_for_reading(Oid varid)
  * 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 effect this function acquires AccessShareLock on
  * related session variable until the end of the transaction.
@@ -710,12 +800,12 @@ SetSessionVariable(Oid varid, Datum value, bool isNull)
 	if (!sessionvars)
 		create_sessionvars_hashtables();
 
-	/* Ensure so all entries in sessionvars hash table are valid */
-	sync_sessionvars_all();
-
 	/* Protect used session variable against drop until transaction end */
 	LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);
 
+	/* Ensure so all entries in sessionvars hash table are valid */
+	sync_sessionvars_all();
+
 	svar = (SVariable) hash_search(sessionvars, &varid,
 								   HASH_ENTER, &found);
 
@@ -733,6 +823,10 @@ SetSessionVariable(Oid varid, Datum value, bool isNull)
 			 varid);
 	}
 
+	/*
+	 * This should either succeed or fail without changing the currently stored
+	 * value.
+	 */
 	set_session_variable(svar, value, isNull, false);
 }
 
@@ -745,7 +839,7 @@ SetSessionVariableWithSecurityCheck(Oid varid, Datum value, bool isNull)
 	AclResult	aclresult;
 
 	/*
-	 * Is possible to write to session variable?
+	 * Is caller allowed to update the session variable?
 	 */
 	aclresult = pg_variable_aclcheck(varid, GetUserId(), ACL_UPDATE);
 	if (aclresult != ACLCHECK_OK)
@@ -755,7 +849,7 @@ SetSessionVariableWithSecurityCheck(Oid varid, Datum value, bool isNull)
 }
 
 /*
- * Returns a copy of value of the session variable specified by varid
+ * Returns a copy of the value of the session variable specified by its oid.
  * Caller is responsible for doing permission checks.
  */
 Datum
@@ -769,7 +863,7 @@ CopySessionVariable(Oid varid, bool *isNull, Oid *typid)
 
 	*typid = svar->typid;
 
-	/* force copy of not null value */
+	/* force copy of non NULL value */
 	if (!svar->isnull)
 	{
 		result = datumCopy(svar->value, svar->typbyval, svar->typlen);
@@ -785,9 +879,9 @@ CopySessionVariable(Oid varid, bool *isNull, Oid *typid)
 }
 
 /*
- * Returns a copy of value of the session variable specified by varid
- * with check of expected type. Like previous function, the caller
- * is responsible for doing permission checks.
+ * Returns a copy of ths value of the session variable specified by its oid
+ * with a check of the expected type. Like previous CopySessionVariable, the
+ * caller is responsible for doing permission checks.
  */
 Datum
 CopySessionVariableWithTypeCheck(Oid varid, bool *isNull, Oid expected_typid)
@@ -818,9 +912,9 @@ CopySessionVariableWithTypeCheck(Oid varid, bool *isNull, Oid expected_typid)
 }
 
 /*
- * Returns a value of session variable identified by varid with
- * check of expected type. Like previous function, the called
- * is reposible for doing permission check.
+ * Returns the value of the session variable specified by its oid  with a check
+ * of the expected type. Like CopySessionVariable, the caller is responsible
+ * for doing permission checks.
  */
 Datum
 GetSessionVariableWithTypeCheck(Oid varid, bool *isNull, Oid expected_typid)
@@ -894,15 +988,15 @@ SessionVariableDropPostprocess(Oid varid)
 			svar->drop_lxid = MyProc->lxid;
 
 			/*
-			 * For variables that are not purged by default we need to
+			 * For variables that are not ON TRANSACTION END RESET, we need to
 			 * register an SVAR_ON_COMMIT_RESET action to free the local
-			 * memory for this variable when this transaction or
-			 * subtransaction is committed (we don't need to wait for sinval
-			 * message).  The cleaning action for one session variable can be
-			 * repeated in the action list without causing any problem, so we
+			 * memory for this variable when the top level transaction
+			 * is committed (we don't need to wait for sinval
+			 * message).  The cleanup action for one session variable can be
+			 * duplicated 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
+			 * but we want to execute this cleanup only when the current
 			 * transaction will be committed. This action can be reverted by
 			 * ABORT of DROP VARIABLE command.
 			 */
@@ -914,10 +1008,9 @@ SessionVariableDropPostprocess(Oid varid)
 }
 
 /*
- * Fast drop of the complete content of all session variables hash table.
- * This is code for DISCARD VARIABLES command. This command
- * cannot be run inside transaction, so we don't need to handle
- * end of transaction actions.
+ * Fast drop of the complete content of all session variables hash table, and
+ * cleanup of any list that wouldn't be relevant anymore.
+ * This is used by DISCARD VARIABLES (and DISCARD ALL) command.
  */
 void
 ResetSessionVariables(void)
@@ -927,9 +1020,6 @@ ResetSessionVariables(void)
 	{
 		hash_destroy(sessionvars);
 		sessionvars = NULL;
-
-		hash_destroy(sessionvars_types);
-		sessionvars_types = NULL;
 	}
 
 	/* Release memory allocated by session variables */
@@ -953,65 +1043,10 @@ ResetSessionVariables(void)
 }
 
 /*
- * 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 clean (reset) local memory allocated by
- * values of dropped session variables.
- */
-static void
-register_session_variable_xact_action(Oid varid,
-									  SVariableXActAction action)
-{
-	SVariableXActActionItem *xact_ai;
-	MemoryContext oldcxt;
-
-	elog(DEBUG1, "SVariableXActAction \"%s\" is registered for session variable (oid:%u)",
-		 SvariableXActActionName(action), varid);
-
-	oldcxt = MemoryContextSwitchTo(TopTransactionContext);
-
-	xact_ai = (SVariableXActActionItem *)
-		palloc(sizeof(SVariableXActActionItem));
-
-	xact_ai->varid = varid;
-	xact_ai->action = action;
-
-	xact_ai->creating_subid = GetCurrentSubTransactionId();
-	xact_ai->deleting_subid = InvalidSubTransactionId;
-
-	xact_on_commit_actions = lcons(xact_ai, xact_on_commit_actions);
-
-	MemoryContextSwitchTo(oldcxt);
-}
-
-/*
- * 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,
-										SVariableXActAction action)
-{
-	ListCell   *l;
-
-	elog(DEBUG1, "SVariableXActAction \"%s\" is unregistered for session variable (oid:%u)",
-		 SvariableXActActionName(action), varid);
-
-	foreach(l, xact_on_commit_actions)
-	{
-		SVariableXActActionItem *xact_ai =
-		(SVariableXActActionItem *) lfirst(l);
-
-		if (xact_ai->action == action && xact_ai->varid == varid)
-			xact_ai->deleting_subid = GetCurrentSubTransactionId();
-	}
-}
-
-/*
- * Perform ON TRANSACTION END RESET or ON COMMIT DROP
- * and COMMIT/ROLLBACK of transaction session variables.
+ * Perform the necessary work for ON TRANSACTION END RESET and ON COMMIT DROP
+ * session variables.
+ * If the transaction is committed, also process the delayed memory cleanup of
+ * local DROP VARIABLE and process all pending rechecks.
  */
 void
 AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
@@ -1019,15 +1054,15 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
 	ListCell   *l;
 
 	/*
-	 * Clean memory for all eox_reset variables. Do it first, it reduces
-	 * enhancing action lists about RECHECK action.
+	 * Clean memory for all ON TRANSACTION END RESET  variables. Do it first,
+	 * as it reduces the overhead of the RECHECK action list.
 	 */
 	foreach(l, xact_reset_varids)
 	{
 		remove_session_variable_by_id(lfirst_oid(l));
 	}
 
-	/* We can clean xact_reset_varids */
+	/* We can now clean xact_reset_varids */
 	list_free(xact_reset_varids);
 	xact_reset_varids = NIL;
 
@@ -1036,50 +1071,57 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
 		foreach(l, xact_on_commit_actions)
 		{
 			SVariableXActActionItem *xact_ai =
-			(SVariableXActActionItem *) lfirst(l);
+				(SVariableXActActionItem *) lfirst(l);
 
 			/* Iterate only over entries that are still pending */
-			if (xact_ai->deleting_subid == InvalidSubTransactionId)
+			if (xact_ai->deleting_subid != InvalidSubTransactionId)
+				continue;
+
+			/*
+			 * ON COMMIT DROP is allowed only for temp session variables.
+			 * So we should explicitly delete only when the current
+			 * transaction is committed. When it's rollbacked, the session
+			 * variable is removed automatically.
+			 */
+			if (xact_ai->action == SVAR_ON_COMMIT_DROP)
 			{
-				if (xact_ai->action == SVAR_ON_COMMIT_DROP)
-				{
-					ObjectAddress object;
-
-					/*
-					 * ON COMMIT DROP is allowed only for temp session variables.
-					 * So we should explicitly delete only when current
-					 * transaction was committed. When it's rollback, then session
-					 * variable is removed automatically.
-					 */
-
-					object.classId = VariableRelationId;
-					object.objectId = xact_ai->varid;
-					object.objectSubId = 0;
-
-					/*
-					 * Since this is an automatic drop, rather than one directly
-					 * initiated by the user, we pass the
-					 * PERFORM_DELETION_INTERNAL flag.
-					 */
-					elog(DEBUG1, "session variable (oid:%u) will be deleted (forced by SVAR_ON_COMMIT_DROP action)",
-						 xact_ai->varid);
-
-					performDeletion(&object, DROP_CASCADE,
-									PERFORM_DELETION_INTERNAL |
-									PERFORM_DELETION_QUIETLY);
-				}
-				else
-				{
-					/*
-					 * When we process DROP VARIABLE statement, we create
-					 * SVAR_ON_COMMIT_RESET xact action. We want to process this
-					 * action only when related transaction is commited (when DROP
-					 * VARIABLE statement sucessfully processed). We want to preserve
-					 * variable content, when the transaction with DROP VARAIBLE
-					 * statement was reverted.
-					 */
-					remove_session_variable_by_id(xact_ai->varid);
-				}
+				ObjectAddress object;
+
+				object.classId = VariableRelationId;
+				object.objectId = xact_ai->varid;
+				object.objectSubId = 0;
+
+				/*
+				 * Since this is an automatic drop, rather than one directly
+				 * initiated by the user, we pass the
+				 * PERFORM_DELETION_INTERNAL flag.
+				 */
+				elog(DEBUG1, "session variable (oid:%u) will be deleted (forced by SVAR_ON_COMMIT_DROP action)",
+					 xact_ai->varid);
+
+				/*
+				 * If the variable was locally set, the memory will be
+				 * automatically cleaned up when we process the underlying
+				 * shared invalidation for this drop.  There can't be a recheck
+				 * action for this variable, so there's nothing to gain
+				 * explicitly removing it here.
+				 */
+				performDeletion(&object, DROP_CASCADE,
+								PERFORM_DELETION_INTERNAL |
+								PERFORM_DELETION_QUIETLY);
+			}
+			else
+			{
+				/*
+				 * When we process DROP VARIABLE statement issued by the
+				 * current transaction, we create an SVAR_ON_COMMIT_RESET xact
+				 * action. We want to process this action only when related
+				 * transaction is commited (when DROP VARIABLE statement
+				 * sucessfully processed) as we need to preserve the variable
+				 * content if the transaction that issued the DROP VARAIBLE
+				 * statement is rollbacked.
+				 */
+				remove_session_variable_by_id(xact_ai->varid);
 			}
 		}
 	}
@@ -1091,6 +1133,13 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
 	list_free_deep(xact_on_commit_actions);
 	xact_on_commit_actions = NIL;
 
+	/*
+	 * We process the list of recheck last for performance reason,the previous
+	 * steps might remove entries from the hash table.
+	 * We need catalog access to process the recheck, so this can only be done
+	 * if the transaction is committed.  Otherwise, we just keep the recheck
+	 * list as-is and it will be processed at the next (committed) transaction.
+	 */
 	if (isCommit && xact_recheck_varids)
 	{
 		Assert(sessionvars);
@@ -1117,14 +1166,15 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
 }
 
 /*
- * Post-subcommit or post-subabort cleanup of xact action list.
+ * Post-subcommit or post-subabort cleanup of xact_on_commit_actions list.
  *
  * During subabort, we can immediately remove entries created during this
  * subtransaction. During subcommit, just transfer entries marked during
  * this subtransaction as being the parent's responsibility.
  */
 void
-AtEOSubXact_SessionVariable_on_xact_actions(bool isCommit, SubTransactionId mySubid,
+AtEOSubXact_SessionVariable_on_xact_actions(bool isCommit,
+											SubTransactionId mySubid,
 											SubTransactionId parentSubid)
 {
 	ListCell   *cur_item;
@@ -1134,19 +1184,24 @@ AtEOSubXact_SessionVariable_on_xact_actions(bool isCommit, SubTransactionId mySu
 		SVariableXActActionItem *xact_ai =
 		(SVariableXActActionItem *) lfirst(cur_item);
 
+		/*
+		 * The subtransaction that created this entry was rollbacked, we can
+		 * remove it.
+		 */
 		if (!isCommit && xact_ai->creating_subid == mySubid)
 		{
-			/* cur_item must be removed */
 			xact_on_commit_actions = foreach_delete_current(xact_on_commit_actions, cur_item);
 			pfree(xact_ai);
 		}
 		else
 		{
-			/* cur_item must be preserved */
+			/* Otherwise cur_item must be preserved */
 			if (xact_ai->creating_subid == mySubid)
 				xact_ai->creating_subid = parentSubid;
+
 			if (xact_ai->deleting_subid == mySubid)
-				xact_ai->deleting_subid = isCommit ? parentSubid : InvalidSubTransactionId;
+				xact_ai->deleting_subid = isCommit ? parentSubid
+												   : InvalidSubTransactionId;
 		}
 	}
 }
@@ -1154,7 +1209,10 @@ AtEOSubXact_SessionVariable_on_xact_actions(bool isCommit, SubTransactionId mySu
 /*
  * pg_debug_show_used_session_variables - designed for testing
  *
- * returns content of session vars
+ * This is a function designed for testing and debugging.  It returns the
+ * content of sessionvars as-is, and can therefore display entries about
+ * session variables that were dropped but for which this backend didn't
+ * process the shared invalidations yet.
  */
 Datum
 pg_debug_show_used_session_variables(PG_FUNCTION_ARGS)
@@ -1224,8 +1282,8 @@ pg_debug_show_used_session_variables(PG_FUNCTION_ARGS)
 			else
 			{
 				/*
-				 * When session variable was removed from catalog, but still
-				 * it in memory. The memory was not purged yet.
+				 * When session variable was removed from catalog, but we
+				 * haven't processed the invlidation yet.
 				 */
 				nulls[1] = true;
 				nulls[2] = true;
@@ -1236,7 +1294,6 @@ pg_debug_show_used_session_variables(PG_FUNCTION_ARGS)
 				nulls[8] = true;
 			}
 
-
 			tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
 		}
 	}
diff --git a/src/include/commands/session_variable.h b/src/include/commands/session_variable.h
index 3912ce39ef..aae3d25112 100644
--- a/src/include/commands/session_variable.h
+++ b/src/include/commands/session_variable.h
@@ -1,7 +1,7 @@
 /*-------------------------------------------------------------------------
  *
- * sessionvariable.h
- *	  prototypes for sessionvariable.c.
+ * session_variable.h
+ *	  prototypes for session_variable.c.
  *
  *
  * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
-- 
2.37.0

