public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v20220916 04/13] FIXUP 0003-session-variables 5+ messages / 4 participants [nested] [flat]
* [PATCH v20220916 04/13] FIXUP 0003-session-variables @ 2022-09-15 15:33 Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Julien Rouhaud @ 2022-09-15 15:33 UTC (permalink / raw) --- 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" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Incorrect result of bitmap heap scan. @ 2024-12-02 17:07 Peter Geoghegan <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Peter Geoghegan @ 2024-12-02 17:07 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Matthias van de Meent <[email protected]>; Konstantin Knizhnik <[email protected]>; pgsql-hackers On Mon, Dec 2, 2024 at 12:02 PM Tom Lane <[email protected]> wrote: > Andres Freund <[email protected]> writes: > > I think the problematic scenario involves tuples that *nobody* can see. During > > the bitmap index scan we don't know that though. Thus the tid gets inserted > > into the bitmap. Then, before we visit the heap, a concurrent vacuum removes > > the tuple from the indexes and then the heap and marks the page as > > all-visible, as the deleted row version has been removed. > > Yup. I am saying that that qualifies as too-aggressive setting of the > all-visible bit. I'm not sure what rule we should adopt instead of > the current one, but I'd much rather slow down page freezing than > institute new page locking rules. Freezing a page, and setting a page all-visible are orthogonal. Nothing has changed about when or how we set pages all-visible in a long time -- VACUUM has always done that to the maximum extent that its OldestXmin cutoff will allow. (The changes to freezing made freezing work a little bit more like that, the general idea being to batch-up work.) -- Peter Geoghegan ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Incorrect result of bitmap heap scan. @ 2024-12-02 17:11 Tom Lane <[email protected]> parent: Peter Geoghegan <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2024-12-02 17:11 UTC (permalink / raw) To: Peter Geoghegan <[email protected]>; +Cc: Andres Freund <[email protected]>; Matthias van de Meent <[email protected]>; Konstantin Knizhnik <[email protected]>; pgsql-hackers Peter Geoghegan <[email protected]> writes: > On Mon, Dec 2, 2024 at 12:02 PM Tom Lane <[email protected]> wrote: >> Yup. I am saying that that qualifies as too-aggressive setting of the >> all-visible bit. I'm not sure what rule we should adopt instead of >> the current one, but I'd much rather slow down page freezing than >> institute new page locking rules. > Freezing a page, and setting a page all-visible are orthogonal. Sorry, sloppy wording on my part. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Incorrect result of bitmap heap scan. @ 2024-12-02 18:39 Peter Geoghegan <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Peter Geoghegan @ 2024-12-02 18:39 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Matthias van de Meent <[email protected]>; Konstantin Knizhnik <[email protected]>; pgsql-hackers On Mon, Dec 2, 2024 at 12:11 PM Tom Lane <[email protected]> wrote: > > Freezing a page, and setting a page all-visible are orthogonal. > > Sorry, sloppy wording on my part. Freezing doesn't affect the contents of the visibility map in any way that seems relevant. The executor only cares about the all-visible bit (and never the all-frozen bit), and the rules around when and how VACUUM sets the all-visible bit (and how everybody else unsets the all-visible bit) haven't changed in forever. So I just can't see it. I guess it's natural to suspect more recent work -- commit 7c70996e is about 6 years old. But I the race condition that I suspect is at play here is very narrow. It's pretty unlikely that there'll be a dead-to-all TID returned to a scan (not just dead to our MVCC snapshot, dead to everybody's) that is subsequently concurrently removed from the index, and then set LP_UNUSED in the heap. It's probably impossible if you don't have a small table -- VACUUM just isn't going to be fast enough to get to the leaf page after the bitmap index scan, but still be able to get to the heap before its corresponding bitmap heap scan (that uses the VM as an optimization) can do the relevant visibility checks (while it could happen with a large table and a slow bitmap scan, the chances of the VACUUM being precisely aligned with the bitmap scan, in just the wrong way, seem remote in the extreme). Finally, none of this will happen if some other factor hinders VACUUM from setting the relevant heap page all-visible. AFAICT this is only a problem because of the involvement of the VM, specifically -- an MVCC snapshot *is* generally sufficient to make bitmap index scans safe from the dangers of concurrent TID recycling, as explained in "62.4. Index Locking Considerations". That only ceases to be true when the visibility map becomes involved (the VM lacks the granular visibility information required to make all this safe). This is essentially the same VM race issue that nbtree's _bt_drop_lock_and_maybe_pin protects against during conventional index-only scans. -- Peter Geoghegan ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Incorrect result of bitmap heap scan. @ 2024-12-02 20:55 Andres Freund <[email protected]> parent: Peter Geoghegan <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Andres Freund @ 2024-12-02 20:55 UTC (permalink / raw) To: Peter Geoghegan <[email protected]>; +Cc: Tom Lane <[email protected]>; Matthias van de Meent <[email protected]>; Konstantin Knizhnik <[email protected]>; pgsql-hackers Hi, On 2024-12-02 13:39:43 -0500, Peter Geoghegan wrote: > I guess it's natural to suspect more recent work -- commit 7c70996e is > about 6 years old. But I the race condition that I suspect is at play > here is very narrow. FWIW, the test I just posted shows the issue down to 11 (although for 11 one has to remove the (TRUNCATE false). 10 returns correct results. I don't think the race is particularly narrow. Having a vacuum complete between the start of the bitmap indexscan and the end of the bitmap heapscan, leaves a lot of time with an expensive query. I suspect one contributor to this avoiding attention till now was that the optimization is fairly narrow: /* * We can potentially skip fetching heap pages if we do not need * any columns of the table, either for checking non-indexable * quals or for returning data. This test is a bit simplistic, as * it checks the stronger condition that there's no qual or return * tlist at all. But in most cases it's probably not worth working * harder than that. */ need_tuples = (node->ss.ps.plan->qual != NIL || node->ss.ps.plan->targetlist != NIL); Even an entry in the targetlist that that does not depend on the current row disables the optimization. Due to not being able to return any content for those rows, it's also somewhat hard to actually notice that the results are wrong... > It's pretty unlikely that there'll be a dead-to-all TID returned to a > scan (not just dead to our MVCC snapshot, dead to everybody's) that is > subsequently concurrently removed from the index, and then set > LP_UNUSED in the heap. It's probably impossible if you don't have a > small table -- VACUUM just isn't going to be fast enough to get to the > leaf page after the bitmap index scan, but still be able to get to the > heap before its corresponding bitmap heap scan (that uses the VM as an > optimization) can do the relevant visibility checks (while it could > happen with a large table and a slow bitmap scan, the chances of the > VACUUM being precisely aligned with the bitmap scan, in just the wrong > way, seem remote in the extreme). A cursor, waiting for IO, waiting for other parts of the query, ... can all make this windows almost arbitrarily large. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-12-02 20:55 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-09-15 15:33 [PATCH v20220916 04/13] FIXUP 0003-session-variables Julien Rouhaud <[email protected]> 2024-12-02 17:07 Re: Incorrect result of bitmap heap scan. Peter Geoghegan <[email protected]> 2024-12-02 17:11 ` Re: Incorrect result of bitmap heap scan. Tom Lane <[email protected]> 2024-12-02 18:39 ` Re: Incorrect result of bitmap heap scan. Peter Geoghegan <[email protected]> 2024-12-02 20:55 ` Re: Incorrect result of bitmap heap scan. Andres Freund <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox