Hi,
On Fri, Sep 02, 2022 at 07:42:00AM +0200, Pavel Stehule wrote:
>
> rebased today
After some off-list discussion with Pavel, I'm sending some proposal patches
(in .txt extension) to apply to the last patchset.
To sum up, when a session issues a DROP VARIABLE, the session will receive an
sinval notification for its own drop and we don't want to process it
immediately as we need to hold the value in case the transaction is rollbacked.
The current patch avoided that by forcing a single processing of sinval per
transaction, and forcing it before dropping the variable. It works but it
seems to me that postponing all but the first VARIABLEOID sinval to the next
transaction is a big hammer, and the sooner we can free some memory the better.
For an alternative approach the attached patch store the lxid in the SVariable
itself when dropping a currently set variable, so we can process all sinval and
simply defer to the next transaction the memory cleanup of the variable(s) we
know we just dropped. What do you think of that approach?
As I was working on some changes I also made a pass on session_variable.c. I
tried to improve a bit some comments, and also got rid of the "first_time"
variable. The name wasn't really great, and AFAICS it can be replaced by
testing whether the memory context has been created yet or not.
But once that done I noticed the get_rowtype_value() function. I don't think
this function is necessary as the core code already knows how to deal with
stored datum when the underlying composite type was modified. I tried to
bypass that function and always simply return the stored value and all the
tests run fine. Is there really any cases when this extra code is needed?
FTR I tried to do a bunch of additional testing using relation as base type for
variable, as you can do more with those than plain composite types, but it
still always works just fine.
However, while doing so I noticed that find_composite_type_dependencies()
failed to properly handle dependencies on relation (plain tables, matviews and
partitioned tables). I'm also adding 2 additional patches to fix this corner
case and add an additional regression test for the plain table case.
>From 43adcd5ed555fd4920061f643c8392c54d8b625e Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouh...@free.fr>
Date: Sat, 3 Sep 2022 16:11:03 +0800
Subject: [PATCH 1/3] FIXUP 0001-catalog-support-for-session-variables
---
src/backend/commands/tablecmds.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d7418e088e..65e1c1e670 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6518,6 +6518,22 @@ find_composite_type_dependencies(Oid typeOid, Relation
origRelation,
RelationGetRelationName(origRelation),
get_namespace_name(get_session_variable_namespace(varid)),
get_session_variable_name(varid))));
+ else if (origRelation->rd_rel->relkind ==
RELKIND_FOREIGN_TABLE)
+ ereport(ERROR,
+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter foreign
table \"%s\" because session variable \"%s.%s\" uses it",
+
RelationGetRelationName(origRelation),
+
get_namespace_name(get_session_variable_namespace(varid)),
+
get_session_variable_name(varid))));
+ else if (origRelation->rd_rel->relkind ==
RELKIND_RELATION ||
+ origRelation->rd_rel->relkind ==
RELKIND_MATVIEW ||
+ origRelation->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
+ ereport(ERROR,
+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter table
\"%s\" because session variable \"%s.%s\" uses it",
+
RelationGetRelationName(origRelation),
+
get_namespace_name(get_session_variable_namespace(varid)),
+
get_session_variable_name(varid))));
}
/* Else, ignore dependees that aren't user columns of relations
*/
--
2.37.0
>From 32ebe63cb31457a6f0e5bf06fa99b91393d6d7c0 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouh...@free.fr>
Date: Sat, 3 Sep 2022 03:13:47 +0800
Subject: [PATCH 2/3] FIXUP 0002-session-variables
---
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
>From 8ecee2a42f77698898eaeeca0a0c269332275e55 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouh...@free.fr>
Date: Sat, 3 Sep 2022 16:09:41 +0800
Subject: [PATCH 3/3] FIXUP 0009-regress-tests-for-session-variables
---
src/test/isolation/specs/session-variable.spec | 2 +-
src/test/regress/expected/session_variables.out | 5 +++++
src/test/regress/sql/session_variables.sql | 6 ++++++
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/test/isolation/specs/session-variable.spec
b/src/test/isolation/specs/session-variable.spec
index a138f0449e..fe47339110 100644
--- a/src/test/isolation/specs/session-variable.spec
+++ b/src/test/isolation/specs/session-variable.spec
@@ -1,4 +1,4 @@
-#
+# Test session variables memory cleanup for sinval
setup
{
diff --git a/src/test/regress/expected/session_variables.out
b/src/test/regress/expected/session_variables.out
index 5e1e8c5dd0..99e433a762 100644
--- a/src/test/regress/expected/session_variables.out
+++ b/src/test/regress/expected/session_variables.out
@@ -531,6 +531,11 @@ EXPLAIN (costs off) LET zero = (SELECT count(*) FROM
svar_test);
-> Parallel Seq Scan on svar_test
(8 rows)
+-- test for dependency on relation
+CREATE VARIABLE v_table AS svar_test;
+ALTER TABLE svar_test ALTER COLUMN a TYPE text;
+ERROR: cannot alter table "svar_test" because session variable
"svartest.v_table" uses it
+DROP VARIABLE v_table;
DROP TABLE svar_test;
DROP VARIABLE zero;
RESET parallel_setup_cost;
diff --git a/src/test/regress/sql/session_variables.sql
b/src/test/regress/sql/session_variables.sql
index 23d0ae716c..d05c28d3eb 100644
--- a/src/test/regress/sql/session_variables.sql
+++ b/src/test/regress/sql/session_variables.sql
@@ -356,6 +356,12 @@ SELECT zero;
-- parallel workers should be used
EXPLAIN (costs off) LET zero = (SELECT count(*) FROM svar_test);
+-- test for dependency on relation
+CREATE VARIABLE v_table AS svar_test;
+
+ALTER TABLE svar_test ALTER COLUMN a TYPE text;
+
+DROP VARIABLE v_table;
DROP TABLE svar_test;
DROP VARIABLE zero;
--
2.37.0
