Thread
-
[PATCH v20220916 04/13] FIXUP 0003-session-variables
Julien Rouhaud <julien.rouhaud@free.fr> — 2022-09-15T15:33:42Z
--- 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 --w2in66fnadvggvsb Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="v20220916-0005-LET-command.patch"