From aa5b0ac4399f2d19827cfa8d5feeb509b58afa37 Mon Sep 17 00:00:00 2001 From: Julien Rouhaud Date: Wed, 2 Feb 2022 15:43:24 +0800 Subject: [PATCH] Few fixups --- src/backend/commands/sessionvariable.c | 100 ++++++++++-------- src/backend/utils/cache/lsyscache.c | 6 +- .../regress/expected/session_variables.out | 2 +- src/test/regress/sql/session_variables.sql | 2 +- 4 files changed, 59 insertions(+), 51 deletions(-) diff --git a/src/backend/commands/sessionvariable.c b/src/backend/commands/sessionvariable.c index 91c8415b9a..0d38acabde 100644 --- a/src/backend/commands/sessionvariable.c +++ b/src/backend/commands/sessionvariable.c @@ -40,39 +40,44 @@ #include "utils/syscache.h" /* - * An values of session variables are stored in local memory - * in sessionvars hash table. This local memory have to be - * cleaned, when a) session variable is dropped by current or - * by other session, b) when user enforce it by using clause - * ON TRANSACTION END RESET. The life cycle of temporal session - * variable can be limmited by using clause ON COMMIT DROP. + * Values of session variables are stored in local memory, in + * sessionvars hash table. This local memory has to be cleaned, + * when: + * - a session variable is dropped by the current or another + * session + * - a user enforce it by using the ON TRANSACTION END RESET + * clause. The life cycle of temporary session variable can be + * limmited by using clause ON COMMIT DROP. * - * Although session variables are not transactional, we don't - * want (and we cannot) to run cleaning immediately (when we - * got sinval message). Session variables are protected by - * AccessShareLock, and then there is not risk of unwanted - * cleaning started by drop variable in second session. But - * AccessShareLock doesn't protect against drop of session - * variable in current session. Without delayed cleaning we - * lost the value if drop command is reverted. Check if - * variable is still valid requires access to system catalog, - * and it can be done only in transaction state). + * Although session variables are not transactional, we don't want + * (and cannot) clean the entries in sessionvars hash table + * immediately, when we get the sinval message. Session variables + * usage is protected by heavyweight locks, so there is no risk of + * unwanted invalidation due to a drop variable done in a + * different session. But it's still possible to drop the session + * variable in the current session. Without delayed cleanup we + * would lose the value if the drop command is done in a sub + * transaction that is then rollbacked. The check of session + * variable validity requires access to system catalog, so it can + * only be done in transaction state). * - * This is reason why cleaning memory (session variable reset) - * is postponed to end of transaction, and we need to hold - * some actions lists. We have to hold two separate action - * lists - one for dropping from system catalog, and second - * for resetting. It's necessary, because dropping session - * variable enforces session variable reset. The drop operation - * can be executed when we iterate over action list, and in - * this moment we would not to modify same action list. + * This is why memory cleanup (session variable reset) is + * postponed to the end of transaction, and why we need to hold + * some actions lists. We have to hold two separate action lists: + * one for dropping the session variable from system catalog, and + * another one for resetting its value. Both are necessary, since + * dropping a session variable also needs to enforce a reset of + * the value. The drop operation can be executed when we iterate + * over the action list, and at that moment we shouldn't modify + * the action list. * - * We want to support possibility to reset session variable at - * the end of transaction. This ensure initial state of session - * variable at the begin of each transaction. The reset is - * implemented like removing variable from sessionvars hash table. - * This enforce full initialization in next usage. - * Attention - this is not same as drop session variable. + * We want to support the possibility of resetting a session + * variable at the end of transaction. This ensures the initial + * state of session variables at the begin of each transaction. + * The reset is implemented as a removal of the session variable + * from sessionvars hash table. This enforce full initialization + * in the next usage. Careful though, this is not same as + * dropping the session variable. * * Another functionality is dropping temporary session variable * with the option ON COMMIT DROP. @@ -82,7 +87,7 @@ typedef enum SVariableXActAction ON_COMMIT_DROP, /* used for ON COMMIT DROP */ ON_COMMIT_RESET, /* used for DROP VARIABLE */ RESET, /* used for ON TRANSACTION END RESET */ - RECHECK /* recheck if session variable is living */ + RECHECK /* verify if session variable still exists */ } SVariableXActAction; typedef struct SVariableXActActionItem @@ -205,25 +210,25 @@ static void pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue) { /* - * We can have not sessionvars in initialized state, and - * we can get this message. This is possible after execution of - * DISCARD VARIABLES command. + * There is no guarantee of sessionvars being initialized, even when + * receiving an invalidation callback, as DISCARD [ ALL | VARIABLES ] + * destroys the hash table entirely. */ if (!sessionvars) return; /* - * When we know so all session variables should be synchronized, - * then is useless to continue; + * Bail out if we know that all session variables should be synchronized, + * as it will be processed by sync_sessionvars_xact_callback. */ if (sync_sessionvars_all) return; /* - * Because we cannot to decode varid from hashValue, we should - * to iterate over all currently used session variables to find - * session variable with same hashValue. On second hand, this can - * save us some CPU later, because we don't need to check any used + * Since we can't guarantee the exact session variable from its hashValue, + * we have to iterate over all currently known session variables to find + * the ones with the same hashValue. On second hand, this can save us + * some CPU later, because we don't need to check any used * session variable (by current session) against system catalog. */ if (hashvalue != 0) @@ -250,8 +255,8 @@ pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue) } /* - * When we need to recheck all session variables, then - * the most effective method is seq scan over hash tab. + * Recheck all session variables. The most effective method is a seq scan over + * the hash table. */ static void sync_sessionvars_xact_callback(XactEvent event, void *arg) @@ -263,10 +268,11 @@ sync_sessionvars_xact_callback(XactEvent event, void *arg) return; /* - * sessionvars is null after DISCARD VARIABLES. - * When we are sure, so there are not any - * active session variable in this session, we - * can reset sync_sessionvars_all flag. + * There is no guarantee of sessionvars being initialized, even when + * receiving an invalidation callback, as DISCARD [ ALL | VARIABLES ] + * destroys the hash table entirely. When there are not any active session + * variable in this session, we can simply reset sync_sessionvars_all flag. + * */ if (!sessionvars) { @@ -343,7 +349,7 @@ create_sessionvars_hashtable(void) /* * Assign some content to the session variable. It's copied to - * SVariableMemoryContext if it is necessary. + * SVariableMemoryContext if necessary. * * init_mode is true, when the value of session variable is initialized * by default expression or by null. Only in this moment we can allow to diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c index da0054c72e..aca8ae8408 100644 --- a/src/backend/utils/cache/lsyscache.c +++ b/src/backend/utils/cache/lsyscache.c @@ -3586,7 +3586,8 @@ get_varname_varid(const char *varname, Oid varnamespace) } /* - * Returns name of session variable specified by oid + * get_session_variable_name + * Returns the name of a given session variable. */ char * get_session_variable_name(Oid varid) @@ -3610,7 +3611,8 @@ get_session_variable_name(Oid varid) } /* - * Returns oid of schema of session variable specified by oid + * get_session_variable_namespace + * Returns the pg_namespace OID associated with a given session variable. */ Oid get_session_variable_namespace(Oid varid) diff --git a/src/test/regress/expected/session_variables.out b/src/test/regress/expected/session_variables.out index 4f25c7f0ba..87eca34d62 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -688,7 +688,7 @@ SELECT count(*) FROM pg_variable WHERE varname = 'xxx_var'; 0 (1 row) --- creating, dropping temporal variable +-- creating, dropping temporary variable BEGIN; CREATE TEMP VARIABLE tempvar AS INT ON COMMIT DROP; LET tempvar = 100; diff --git a/src/test/regress/sql/session_variables.sql b/src/test/regress/sql/session_variables.sql index 3db0b5655e..44ae7e4bd5 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -474,7 +474,7 @@ DROP OWNED BY var_test_role2; DROP ROLE var_test_role2; SELECT count(*) FROM pg_variable WHERE varname = 'xxx_var'; --- creating, dropping temporal variable +-- creating, dropping temporary variable BEGIN; CREATE TEMP VARIABLE tempvar AS INT ON COMMIT DROP; -- 2.33.1