Hi,
On Sun, Jan 30, 2022 at 08:09:18PM +0100, Pavel Stehule wrote:
>
> rebase after 02b8048ba5dc36238f3e7c3c58c5946220298d71
Here are a few comments, mostly about pg_variable.c and sessionvariable.c. I
stopped before reading the whole patch as I have some concern about the sinval
machanism, which ould change a bit the rest of the patch. I'm also attaching a
patch (with .txt extension to avoid problem with the cfbot) with some comment
update propositions.
In sessionvariable.c, why VariableEOXAction and VariableEOXActionCodes? Can't
the parser emit directly the char value, like e.g. relpersistence?
extraneous returns for 2 functions:
+void
+get_session_variable_type_typmod_collid(Oid varid, Oid *typid, int32 *typmod,
+ Oid *collid)
+{
[...]
+ return;
+}
+void
+initVariable(Variable *var, Oid varid, bool fast_only)
+{
[...]
+ return;
+}
VariableCreate():
Maybe add a bunch of AssertArg() for all the mandatory parametrers?
Also, the check for variable already existing should be right after the
AssertArg(), and using SearchSysCacheExistsX().
Maybe also adding an Assert(OidIsValid(xxxoid)) just after the
CatalogTupleInsert(), similarly to some other creation functions?
event-triggers.sgml needs updating for the firing matrix, as session variable
are compatible with even triggers.
+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 */
+} SVariableXActAction;
The names seem a bit generic, maybe add a prefix like SVAR_xxx?
ON_COMMIT_RESET is also confusing as it looks like an SQL clause. Maybe
PERFORM_DROP or something?
+static List *xact_drop_actions = NIL;
+static List *xact_reset_actions = NIL;
Maybe add a comment saying both are lists of SVariableXActAction?
+typedef SVariableData * SVariable;
looks like a missing bump to typedefs.list.
+char *
+get_session_variable_name(Oid varid)
+{
+ HeapTuple tup;
+ Form_pg_variable varform;
+ char *varname;
+
+ tup = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(varid));
+
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for session variable %u", varid);
+
+ varform = (Form_pg_variable) GETSTRUCT(tup);
+
+ varname = NameStr(varform->varname);
+
+ ReleaseSysCache(tup);
+
+ return varname;
+}
This kind of function should return a palloc'd copy of the name.
+void
+ResetSessionVariables(void)
[...]
+ list_free_deep(xact_drop_actions);
+ xact_drop_actions = NIL;
+
+ list_free_deep(xact_reset_actions);
+ xact_drop_actions = NIL;
+}
The 2nd chunk should be xact_reset_actions = NIL
+static void register_session_variable_xact_action(Oid varid,
SVariableXActAction action);
+static void delete_session_variable_xact_action(Oid varid, SVariableXActAction
action);
The naming is a bit confusing, maybe unregister_session_cable_xact_action() for
consistency?
+void
+register_session_variable_xact_action(Oid varid,
+ SVariableXActAction action)
the function is missing the static keyword.
In AtPreEOXact_SessionVariable_on_xact_actions(), those 2 instructions are
executed twice (once in the middle and once at the end):
list_free_deep(xact_drop_actions);
xact_drop_actions = NIL;
+ * If this entry was created during the current transaction,
+ * creating_subid is the ID of the creating subxact; if created in a prior
+ * transaction, creating_subid is zero.
I don't see any place in the code where creating_subid can be zero? It looks
like it's only there for future transactional implementation, but for now this
attribute seems unnecessary?
/* at transaction end recheck sinvalidated variables */
RegisterXactCallback(sync_sessionvars_xact_callback, NULL);
I don't think it's ok to use xact callback for in-core code. The function
explicitly says:
> * These functions are intended for use by dynamically loaded modules.
> * For built-in modules we generally just hardwire the appropriate calls
> * (mainly because it's easier to control the order that way, where needed).
Also, this function and AtPreEOXact_SessionVariable_on_xact_actions() are
skipping all or part of the processing if there is no active transaction. Is
that really ok?
I'm particularly sceptical about AtPreEOXact_SessionVariable_on_xact_actions
and the RECHECK actions, as the xact_reset_actions list is reset whether the
recheck was done or not, so it seems to me that it could be leaking some
entries in the hash table. If the database has a lot of object, it seems
possible (while unlikely) that a subsequent CREATE VARIABLE can get the same
oid leading to incorrect results?
If that's somehow ok, wouldn't it be better to rearrange the code to call those
functions less often, and only when they can do their work, or at least split
the recheck in some different function / list?
+static void
+pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
[...]
+ if (hashvalue != 0)
+ {
[...]
+ }
+ else
+ sync_sessionvars_all = true;
The rechecks being somewhat expensive, I think it could be a win to remove all
pending rechecks when setting the sync_sessionvars_all.
>From aa5b0ac4399f2d19827cfa8d5feeb509b58afa37 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouh...@free.fr>
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
