Thread
-
[PATCH 2/3] FIXUP 0002-session-variables
Julien Rouhaud <julien.rouhaud@free.fr> — 2022-09-02T19:13:47Z
--- src/backend/commands/session_variable.c | 137 +++++++++++++----------- 1 file changed, 73 insertions(+), 64 deletions(-) diff --git a/src/backend/commands/session_variable.c b/src/backend/commands/session_variable.c index d5febfef75..711ebce76e 100644 --- a/src/backend/commands/session_variable.c +++ b/src/backend/commands/session_variable.c @@ -45,25 +45,26 @@ #include "utils/typcache.h" /* - * Values of session variables are stored in local memory, in - * sessionvars hash table in binary format and the life cycle of - * session variable can be longer than transaction. Then we - * need to solve following issues: + * 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: * - * - We need to free local memory when variable is dropped (in current - * transaction in current session or by other sessions (other - * users). There is a request to protect content against - * possibly aborted the DROP VARIABLE command. So the purging should - * not be executed immediately. It should be postponed until the end - * of the transaction. + * - 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. * - * - The session variable can be dropped explicitly (by DROP VARIABLE) - * command or implicitly (by using ON COMMIT DROP clause). The - * purging memory at transaction end can be requested implicitly - * too (by using the ON TRANSACTION END clause). + * - 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. * - * The values of session variables are stored in hash table - * sessionvars. We maintain four queues (implemented as list + * 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 @@ -140,20 +141,15 @@ static List *xact_reset_varids = NIL; /* * When the session variable is dropped we need to free local memory. The * session variable can be dropped by current session, but it can be - * dropped by other's sessions too, so we have to watc sinval message. + * dropped by other's sessions too, so we have to watch sinval message. * But because we don't want to free local memory immediately, we need to * 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. When the check is postponed - * to the next transaction, then have to be executed just once time. - * Saved lxid in synced_lxid is safeguard against repeated useles - * recheck inside one transaction. + * current transactions is in aborted state. */ static List *xact_recheck_varids = NIL; -static LocalTransactionId synced_lxid = InvalidLocalTransactionId; - typedef struct SVariableData { Oid varid; /* pg_variable OID of this sequence (hash key) */ @@ -176,7 +172,8 @@ typedef struct SVariableData * acceptable for debug purposes (and it is necessary, because * sometimes we cannot to access to catalog. */ - char *name; /* session variable name (at time of initialization) */ + char *name; /* session variable name (at time of + initialization) */ char *nsname; /* session variable schema name */ bool isnull; @@ -193,6 +190,13 @@ typedef struct SVariableData void *domain_check_extra; LocalTransactionId domain_check_extra_lxid; + /* + * 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. + */ + LocalTransactionId drop_lxid; + TupleDesc tupdesc; /* for row type we save actual tupdesc * * at save time, for conversion to uptime * * tupdesc in read time */ @@ -217,8 +221,6 @@ static HTAB *sessionvars_types = NULL; /* hash table for type fingerprints of se static MemoryContext SVariableMemoryContext = NULL; -static bool first_time = true; - static void init_session_variable(SVariable svar, Variable *var); static void register_session_variable_xact_action(Oid varid, SVariableXActAction action); @@ -360,7 +362,7 @@ pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue) /* * although it there is low probability, we have to iterate - * over all actively used session variables, because hashvalue + * over all locally set session variables, because hashvalue * is not unique identifier. */ } @@ -397,9 +399,7 @@ 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, but it - * should be called only once per transaction. The synced_lxid - * is used as protection against repeated call. + * any read or any write from/to session variable. */ static void sync_sessionvars_all() @@ -407,14 +407,8 @@ sync_sessionvars_all() SVariable svar; ListCell *l; - if (synced_lxid == MyProc->lxid) - return; - if (!xact_recheck_varids) - { - synced_lxid = MyProc->lxid; return; - } /* * sessionvars is null after DISCARD VARIABLES. @@ -425,14 +419,13 @@ sync_sessionvars_all() { list_free(xact_recheck_varids); xact_recheck_varids = NIL; - synced_lxid = MyProc->lxid; return; } elog(DEBUG1, "effective call of sync_sessionvars_all()"); /* - * This routine is called before any reading. So the + * This routine is called before any reading, so the * session should be in transaction state. This is required * for access to system catalog. */ @@ -446,18 +439,27 @@ sync_sessionvars_all() svar = (SVariable) hash_search(sessionvars, &varid, HASH_FIND, &found); + /* + * 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. + */ if (found) { + /* + * If this is a variable dropped by the current transaction, ignore + * it and keep the oid to recheck in the next transaction. + */ + if (svar->drop_lxid == MyProc->lxid) + continue; + if (!is_session_variable_valid(svar)) remove_session_variable(svar); } - } - - list_free(xact_recheck_varids); - xact_recheck_varids = NIL; - /* Don't repeat this check in this transaction more time */ - synced_lxid = MyProc->lxid; + xact_recheck_varids = foreach_delete_current(xact_recheck_varids, l); + } } /* @@ -468,20 +470,21 @@ create_sessionvars_hashtables(void) { HASHCTL vars_ctl; + Assert(!sessionvars); + /* set callbacks */ - if (first_time) + if (!SVariableMemoryContext) { /* Read sinval messages */ CacheRegisterSyscacheCallback(VARIABLEOID, pg_variable_cache_callback, (Datum) 0); - /* needs its own long lived memory context */ + + /* We need our own long lived memory context */ SVariableMemoryContext = AllocSetContextCreate(TopMemoryContext, "session variables", ALLOCSET_START_SMALL_SIZES); - - first_time = false; } Assert(SVariableMemoryContext); @@ -946,7 +949,6 @@ SetSessionVariable(Oid varid, Datum value, bool isNull) SVariable svar; bool found; - if (!sessionvars) create_sessionvars_hashtables(); @@ -1238,24 +1240,31 @@ RemoveSessionVariable(Oid varid) svar = (SVariable) hash_search(sessionvars, &varid, HASH_FIND, &found); - /* - * For variables that are not purged by default we need to - * register SVAR_ON_COMMIT_RESET action. This action can - * be reverted by ABORT of DROP VARIABLE command. - */ - if (found && !svar->eox_reset) + if (found) { + /* Save the current top level local transaction id to make sure we + * don't automatically remove the local variable storage in + * sync_sessionvars_all, as the DROP VARIABLE will send an + * invalidation message. + */ + Assert(LocalTransactionIdIsValid(MyProc->lxid)); + svar->drop_lxid = MyProc->lxid; + /* - * And we want to enforce variable clearning when this transaction or - * subtransaction will be committed (we don't need to wait for - * sinval message). The cleaning action for one session variable - * can be repeated in the action list without causing any problem, - * so we don't need to ensure uniqueness. We need a different action - * from RESET, because RESET is executed on any transaction end, - * but we want to execute cleaning only when the current transaction - * will be committed. + * For variables that are not purged by default 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 don't need to ensure + * uniqueness. We need a different action from RESET, because + * RESET is executed on any transaction end, but we want to execute + * cleaning only when the current transaction will be committed. + * This action can be reverted by ABORT of DROP VARIABLE command. */ - register_session_variable_xact_action(varid, SVAR_ON_COMMIT_RESET); + if (!svar->eox_reset) + register_session_variable_xact_action(varid, + SVAR_ON_COMMIT_RESET); } } } -- 2.37.0 --vuzhuyepzhyurc52 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0003-FIXUP-0009-regress-tests-for-session-variables.txt"