From 3af2b5d3483521a294f4efe52bb430b432dc96b6 Mon Sep 17 00:00:00 2001 From: Laurenz Albe Date: Wed, 13 Nov 2024 09:31:28 +0100 Subject: [PATCH 05/21] memory cleaning after DROP VARIABLE Accepting a sinval message invalidates entries in the "sessionvars" hash table. These entries are validated before any read or write operations on session variables. When the entry cannot be validated, it is removed. Removal will be delayed when the variable was dropped by the current transaction, which could still be rolled back. --- src/backend/catalog/pg_variable.c | 7 +- src/backend/commands/session_variable.c | 153 +++++++++++- src/include/commands/session_variable.h | 2 + .../isolation/expected/session-variable.out | 110 +++++++++ src/test/isolation/isolation_schedule | 1 + .../isolation/specs/session-variable.spec | 50 ++++ .../regress/expected/session_variables.out | 217 ++++++++++++++++++ src/test/regress/sql/session_variables.sql | 120 ++++++++++ 8 files changed, 655 insertions(+), 5 deletions(-) create mode 100644 src/test/isolation/expected/session-variable.out create mode 100644 src/test/isolation/specs/session-variable.spec diff --git a/src/backend/catalog/pg_variable.c b/src/backend/catalog/pg_variable.c index bb445e9434b..d672d2a9cb3 100644 --- a/src/backend/catalog/pg_variable.c +++ b/src/backend/catalog/pg_variable.c @@ -22,6 +22,7 @@ #include "catalog/pg_collation.h" #include "catalog/pg_namespace.h" #include "catalog/pg_variable.h" +#include "commands/session_variable.h" #include "miscadmin.h" #include "parser/parse_type.h" #include "utils/builtins.h" @@ -230,7 +231,8 @@ CreateVariable(ParseState *pstate, CreateSessionVarStmt *stmt) } /* - * Drop variable by OID + * Drop variable by OID, and register the needed session variable + * cleanup. */ void DropVariableById(Oid varid) @@ -250,4 +252,7 @@ DropVariableById(Oid varid) ReleaseSysCache(tup); table_close(rel, RowExclusiveLock); + + /* do the necessary cleanup in local memory, if needed */ + SessionVariableDropPostprocess(varid); } diff --git a/src/backend/commands/session_variable.c b/src/backend/commands/session_variable.c index e1c86966998..1596d94fb03 100644 --- a/src/backend/commands/session_variable.c +++ b/src/backend/commands/session_variable.c @@ -14,6 +14,7 @@ */ #include "postgres.h" +#include "access/xact.h" #include "catalog/pg_variable.h" #include "commands/session_variable.h" #include "executor/svariableReceiver.h" @@ -67,6 +68,14 @@ 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 + * variables dropped by the local backend, in case the operation is rolled + * back. + */ + LocalTransactionId drop_lxid; + /* * Stored value and type description can be outdated when we receive a * sinval message. We then have to check if the stored data are still @@ -83,6 +92,17 @@ static HTAB *sessionvars = NULL; /* hash table for session variables */ static MemoryContext SVariableMemoryContext = NULL; +/* becomes true when we receive a sinval message */ +static bool needs_validation = false; + +/* + * The content of dropped session variables is not removed immediately. We do + * that in the next transaction that reads or writes a session variable. + * "validated_lxid" stores the transaction that performed said validation, so + * that we can avoid repeating the effort. + */ +static LocalTransactionId validated_lxid = InvalidLocalTransactionId; + /* * Callback function for session variable invalidation. */ @@ -114,6 +134,38 @@ pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue) if (hashvalue == 0 || svar->hashvalue == hashvalue) { svar->is_valid = false; + needs_validation = true; + } + } +} + +/* + * Handle the local memory cleanup for a DROP VARIABLE command. + * + * Caller should take care of removing the pg_variable entry first. + */ +void +SessionVariableDropPostprocess(Oid varid) +{ + Assert(LocalTransactionIdIsValid(MyProc->vxid.lxid)); + + if (sessionvars) + { + bool found; + SVariable svar = (SVariable) hash_search(sessionvars, &varid, + HASH_FIND, &found); + + if (found) + { + /* + * Save the current top level local transaction id to make sure we + * won't automatically remove the local variable storage in + * validate_all_session_variables() when the invalidation message + * from DROP VARIABLE arrives. After all, the transaction could + * still be rolled back. + */ + svar->is_valid = false; + svar->drop_lxid = MyProc->vxid.lxid; } } } @@ -167,6 +219,67 @@ is_session_variable_valid(SVariable svar) return result; } +/* + * Check all potentially invalid session variable data in local memory and free + * the memory for all invalid ones. This function is called before any read or + * write of a session variable. Freeing of a variable's memory is postponed if + * the variable has been dropped by the current transaction, since that + * operation could still be rolled back. + * + * It is possible that we receive a cache invalidation message while + * remove_invalid_session_variables() is executing, so we cannot guarantee that + * all entries in "sessionvars" will be set to "is_valid" after the function is + * done. However, we can guarantee that all entries get checked once. + */ +static void +remove_invalid_session_variables(void) +{ + HASH_SEQ_STATUS status; + SVariable svar; + + /* + * The validation requires system catalog access, so the session state + * should be "in transaction". + */ + Assert(IsTransactionState()); + + if (!needs_validation || !sessionvars) + return; + + /* + * Reset the flag before we start the validation. It can be set again + * by concurrently incoming sinval messages. + */ + needs_validation = false; + + elog(DEBUG1, "effective call of validate_all_session_variables()"); + + hash_seq_init(&status, sessionvars); + while ((svar = (SVariable) hash_seq_search(&status)) != NULL) + { + if (!svar->is_valid) + { + if (svar->drop_lxid == MyProc->vxid.lxid) + { + /* try again in the next transaction */ + needs_validation = true; + continue; + } + + if (!is_session_variable_valid(svar)) + { + Oid varid = svar->varid; + + free_session_variable_value(svar); + hash_search(sessionvars, &varid, HASH_REMOVE, NULL); + svar = NULL; + } + else + svar->is_valid = true; + } + } +} + /* * Initialize attributes cached in "svar" */ @@ -196,6 +309,8 @@ setup_session_variable(SVariable svar, Oid varid) svar->domain_check_extra = NULL; svar->domain_check_extra_lxid = InvalidLocalTransactionId; + svar->drop_lxid = InvalidTransactionId; + svar->isnull = true; svar->value = (Datum) 0; @@ -319,22 +434,42 @@ get_session_variable(Oid varid) if (!sessionvars) create_sessionvars_hashtables(); + if (validated_lxid == InvalidLocalTransactionId || + validated_lxid != MyProc->vxid.lxid) + { + /* free the memory from dropped session variables */ + remove_invalid_session_variables(); + + /* don't repeat the above step in the same transaction */ + validated_lxid = MyProc->vxid.lxid; + } + svar = (SVariable) hash_search(sessionvars, &varid, HASH_ENTER, &found); if (found) { + /* + * The session variable could have been dropped by a DROP VARIABLE + * statement in a subtransaction that was later rolled back, which + * means that we may have to work with the data of a variable marked + * as invalid. + */ if (!svar->is_valid) { /* - * If there was an invalidation message, the variable might still be - * valid, but we have to check with the system catalog. + * We have to check the system catalog to see if the variable is + * still valid, even if an invalidation message set it to invalid. + * + * The variable must be validated before it is accessed. The oid + * should be valid, because the related session variable is already + * locked, and remove_invalid_session_variables() would remove + * variables dropped by other transactions. */ if (is_session_variable_valid(svar)) svar->is_valid = true; else - /* if the value cannot be validated, we have to discard it */ - free_session_variable_value(svar); + elog(ERROR, "unexpected state of session variable %u", varid); } } else @@ -395,6 +530,16 @@ SetSessionVariable(Oid varid, Datum value, bool isNull) if (!sessionvars) create_sessionvars_hashtables(); + if (validated_lxid == InvalidLocalTransactionId || + validated_lxid != MyProc->vxid.lxid) + { + /* free the memory from dropped session variables */ + remove_invalid_session_variables(); + + /* don't repeat the above step in the same transaction */ + validated_lxid = MyProc->vxid.lxid; + } + svar = (SVariable) hash_search(sessionvars, &varid, HASH_ENTER, &found); diff --git a/src/include/commands/session_variable.h b/src/include/commands/session_variable.h index a8ca43c87af..d3cc79b5608 100644 --- a/src/include/commands/session_variable.h +++ b/src/include/commands/session_variable.h @@ -21,6 +21,8 @@ #include "tcop/cmdtag.h" #include "utils/queryenvironment.h" +extern void SessionVariableDropPostprocess(Oid varid); + extern void SetSessionVariable(Oid varid, Datum value, bool isNull); extern Datum GetSessionVariable(Oid varid, bool *isNull); diff --git a/src/test/isolation/expected/session-variable.out b/src/test/isolation/expected/session-variable.out new file mode 100644 index 00000000000..0a5579dc7ce --- /dev/null +++ b/src/test/isolation/expected/session-variable.out @@ -0,0 +1,110 @@ +Parsed test spec with 4 sessions + +starting permutation: let val drop val +step let: LET myvar = 'test'; +step val: SELECT myvar; +myvar +----- +test +(1 row) + +step drop: DROP VARIABLE myvar; +step val: SELECT myvar; +ERROR: column "myvar" does not exist + +starting permutation: let val s1 drop val sr1 +step let: LET myvar = 'test'; +step val: SELECT myvar; +myvar +----- +test +(1 row) + +step s1: BEGIN; +step drop: DROP VARIABLE myvar; +step val: SELECT myvar; +ERROR: column "myvar" does not exist +step sr1: ROLLBACK; + +starting permutation: let val dbg drop create dbg val +step let: LET myvar = 'test'; +step val: SELECT myvar; +myvar +----- +test +(1 row) + +step dbg: SELECT schema, name, removed FROM pg_session_variables(); +schema|name |removed +------+-----+------- +public|myvar|f +(1 row) + +step drop: DROP VARIABLE myvar; +step create: CREATE VARIABLE myvar AS text; +step dbg: SELECT schema, name, removed FROM pg_session_variables(); +schema|name|removed +------+----+------- + | |t +(1 row) + +step val: SELECT myvar; +myvar +----- + +(1 row) + + +starting permutation: let val s1 dbg drop create dbg val sr1 +step let: LET myvar = 'test'; +step val: SELECT myvar; +myvar +----- +test +(1 row) + +step s1: BEGIN; +step dbg: SELECT schema, name, removed FROM pg_session_variables(); +schema|name |removed +------+-----+------- +public|myvar|f +(1 row) + +step drop: DROP VARIABLE myvar; +step create: CREATE VARIABLE myvar AS text; +step dbg: SELECT schema, name, removed FROM pg_session_variables(); +schema|name |removed +------+-----+------- +public|myvar|f +(1 row) + +step val: SELECT myvar; +myvar +----- + +(1 row) + +step sr1: ROLLBACK; + +starting permutation: create3 let3 s3 create4 let4 drop4 drop3 inval3 discard sc3 state +step create3: CREATE VARIABLE myvar3 AS text; +step let3: LET myvar3 = 'test'; +step s3: BEGIN; +step create4: CREATE VARIABLE myvar4 AS text; +step let4: LET myvar4 = 'test'; +step drop4: DROP VARIABLE myvar4; +step drop3: DROP VARIABLE myvar3; +step inval3: SELECT COUNT(*) >= 0 FROM pg_foreign_table; +?column? +-------- +t +(1 row) + +step discard: DISCARD VARIABLES; +step sc3: COMMIT; +step state: SELECT varname FROM pg_variable; +varname +------- +myvar +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 143109aa4da..7453685d046 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -115,3 +115,4 @@ test: serializable-parallel-2 test: serializable-parallel-3 test: matview-write-skew test: lock-nowait +test: session-variable diff --git a/src/test/isolation/specs/session-variable.spec b/src/test/isolation/specs/session-variable.spec new file mode 100644 index 00000000000..c864fee4006 --- /dev/null +++ b/src/test/isolation/specs/session-variable.spec @@ -0,0 +1,50 @@ +# Test session variables memory cleanup for sinval + +setup +{ + CREATE VARIABLE myvar AS text; +} + +teardown +{ + DROP VARIABLE IF EXISTS myvar; +} + +session s1 +step s1 { BEGIN; } +step let { LET myvar = 'test'; } +step val { SELECT myvar; } +step dbg { SELECT schema, name, removed FROM pg_session_variables(); } +step sr1 { ROLLBACK; } + +session s2 +step drop { DROP VARIABLE myvar; } +step create { CREATE VARIABLE myvar AS text; } + +session s3 +step s3 { BEGIN; } +step let3 { LET myvar3 = 'test'; } +step create4 { CREATE VARIABLE myvar4 AS text; } +step let4 { LET myvar4 = 'test'; } +step drop4 { DROP VARIABLE myvar4; } +step inval3 { SELECT COUNT(*) >= 0 FROM pg_foreign_table; } +step discard { DISCARD VARIABLES; } +step sc3 { COMMIT; } +step state { SELECT varname FROM pg_variable; } + +session s4 +step create3 { CREATE VARIABLE myvar3 AS text; } +step drop3 { DROP VARIABLE myvar3; } + +# Concurrent drop of a known variable should lead to an error +permutation let val drop val +# Same, but with an explicit transaction +permutation let val s1 drop val sr1 +# Concurrent drop/create of a known variable should lead to empty variable +permutation let val dbg drop create dbg val +# Concurrent drop/create of a known variable should lead to empty variable +# We need a transaction to make sure that we won't accept invalidation when +# calling the dbg step after the concurrent drop +permutation let val s1 dbg drop create dbg val sr1 +# test for DISCARD ALL when all internal queues have actions registered +permutation create3 let3 s3 create4 let4 drop4 drop3 inval3 discard sc3 state diff --git a/src/test/regress/expected/session_variables.out b/src/test/regress/expected/session_variables.out index 79ac40768f8..2783caa8446 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1726,3 +1726,220 @@ SELECT count(*) FROM pg_session_variables(); 0 (1 row) +-- dropped variables should be removed from memory before the next usage +-- of any session variable in the next transaction +LET var1 = 'Ahoj'; +SELECT name, typname, can_select, can_update FROM pg_session_variables(); + name | typname | can_select | can_update +------+-------------------+------------+------------ + var1 | character varying | t | t +(1 row) + +DROP VARIABLE var1; +-- should be zero +SELECT count(*) FROM pg_session_variables() WHERE NOT removed; + count +------- + 0 +(1 row) + +-- the content of the value should be preserved when a variable is dropped +-- by an aborted transaction +CREATE VARIABLE var1 AS varchar; +LET var1 = 'Ahoj'; +BEGIN; +DROP VARIABLE var1; +-- should fail +SELECT var1; +ERROR: column "var1" does not exist +LINE 1: SELECT var1; + ^ +ROLLBACK; +-- should be ok +SELECT var1; + var1 +------ + Ahoj +(1 row) + +-- another test +BEGIN; +DROP VARIABLE var1; +CREATE VARIABLE var1 AS int; +LET var1 = 100; +-- should be ok, result 100 +SELECT var1; + var1 +------ + 100 +(1 row) + +ROLLBACK; +-- should be ok, result 'Ahoj' +SELECT var1; + var1 +------ + Ahoj +(1 row) + +DROP VARIABLE var1; +-- should be zero +SELECT count(*) FROM pg_session_variables() WHERE NOT removed; + count +------- + 0 +(1 row) + +BEGIN; + CREATE VARIABLE var1 AS int; + LET var1 = 100; + SELECT var1; + var1 +------ + 100 +(1 row) + + SELECT name, typname, can_select, can_update FROM pg_session_variables(); + name | typname | can_select | can_update +------+---------+------------+------------ + var1 | integer | t | t +(1 row) + + DROP VARIABLE var1; +COMMIT; +-- should be zero +SELECT count(*) FROM pg_session_variables() WHERE NOT removed; + count +------- + 0 +(1 row) + +BEGIN; + CREATE VARIABLE var1 AS int; + LET var1 = 100; + SELECT var1; + var1 +------ + 100 +(1 row) + + SELECT name, typname, can_select, can_update FROM pg_session_variables(); + name | typname | can_select | can_update +------+---------+------------+------------ + var1 | integer | t | t +(1 row) + + DROP VARIABLE var1; +COMMIT; +-- should be zero +SELECT count(*) FROM pg_session_variables() WHERE NOT removed; + count +------- + 0 +(1 row) + +CREATE VARIABLE var1 AS int; +CREATE VARIABLE var2 AS int; +LET var1 = 10; +LET var2 = 0; +BEGIN; + SAVEPOINT s1; + DROP VARIABLE var1; + -- force cleaning by touching another session variable + SELECT var2; + var2 +------ + 0 +(1 row) + + ROLLBACK TO s1; + SAVEPOINT s2; + DROP VARIABLE var1; + SELECT var2; + var2 +------ + 0 +(1 row) + + ROLLBACK TO s2; +COMMIT; +-- should be ok +SELECT var1; + var1 +------ + 10 +(1 row) + +BEGIN; + SAVEPOINT s1; + DROP VARIABLE var1; + -- force cleaning by touching another session variable + SELECT var2; + var2 +------ + 0 +(1 row) + + ROLLBACK TO s1; + SAVEPOINT s2; + DROP VARIABLE var1; + SELECT var2; + var2 +------ + 0 +(1 row) + +ROLLBACK; +-- should be ok +SELECT var1; + var1 +------ + 10 +(1 row) + +BEGIN; + SAVEPOINT s1; + DROP VARIABLE var1; + -- force cleaning by touching another session variable + SELECT var2; + var2 +------ + 0 +(1 row) + + SAVEPOINT s2; + -- force cleaning by touching another session variable + SELECT var2; + var2 +------ + 0 +(1 row) + + ROLLBACK TO s1; + -- force cleaning by touching another session variable + SELECT var2; + var2 +------ + 0 +(1 row) + +COMMIT; +-- should be ok +SELECT var1; + var1 +------ + 10 +(1 row) + +-- repeated aborted transaction +BEGIN; DROP VARIABLE var1; ROLLBACK; +BEGIN; DROP VARIABLE var1; ROLLBACK; +BEGIN; DROP VARIABLE var1; ROLLBACK; +-- should be ok +SELECT var1; + var1 +------ + 10 +(1 row) + +DROP VARIABLE var1, var2; diff --git a/src/test/regress/sql/session_variables.sql b/src/test/regress/sql/session_variables.sql index 2954759aaed..a624ab8b67f 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -1214,3 +1214,123 @@ DISCARD VARIABLES; -- should be zero again SELECT count(*) FROM pg_session_variables(); + +-- dropped variables should be removed from memory before the next usage +-- of any session variable in the next transaction + +LET var1 = 'Ahoj'; +SELECT name, typname, can_select, can_update FROM pg_session_variables(); +DROP VARIABLE var1; + +-- should be zero +SELECT count(*) FROM pg_session_variables() WHERE NOT removed; + +-- the content of the value should be preserved when a variable is dropped +-- by an aborted transaction +CREATE VARIABLE var1 AS varchar; +LET var1 = 'Ahoj'; +BEGIN; +DROP VARIABLE var1; + +-- should fail +SELECT var1; + +ROLLBACK; + +-- should be ok +SELECT var1; + +-- another test +BEGIN; +DROP VARIABLE var1; +CREATE VARIABLE var1 AS int; +LET var1 = 100; +-- should be ok, result 100 +SELECT var1; +ROLLBACK; +-- should be ok, result 'Ahoj' +SELECT var1; + +DROP VARIABLE var1; + +-- should be zero +SELECT count(*) FROM pg_session_variables() WHERE NOT removed; + +BEGIN; + CREATE VARIABLE var1 AS int; + LET var1 = 100; + SELECT var1; + SELECT name, typname, can_select, can_update FROM pg_session_variables(); + DROP VARIABLE var1; +COMMIT; + +-- should be zero +SELECT count(*) FROM pg_session_variables() WHERE NOT removed; + +BEGIN; + CREATE VARIABLE var1 AS int; + LET var1 = 100; + SELECT var1; + SELECT name, typname, can_select, can_update FROM pg_session_variables(); + DROP VARIABLE var1; +COMMIT; + +-- should be zero +SELECT count(*) FROM pg_session_variables() WHERE NOT removed; + +CREATE VARIABLE var1 AS int; +CREATE VARIABLE var2 AS int; +LET var1 = 10; +LET var2 = 0; +BEGIN; + SAVEPOINT s1; + DROP VARIABLE var1; + -- force cleaning by touching another session variable + SELECT var2; + ROLLBACK TO s1; + SAVEPOINT s2; + DROP VARIABLE var1; + SELECT var2; + ROLLBACK TO s2; +COMMIT; +-- should be ok +SELECT var1; + +BEGIN; + SAVEPOINT s1; + DROP VARIABLE var1; + -- force cleaning by touching another session variable + SELECT var2; + ROLLBACK TO s1; + SAVEPOINT s2; + DROP VARIABLE var1; + SELECT var2; +ROLLBACK; +-- should be ok +SELECT var1; + +BEGIN; + SAVEPOINT s1; + DROP VARIABLE var1; + -- force cleaning by touching another session variable + SELECT var2; + + SAVEPOINT s2; + -- force cleaning by touching another session variable + SELECT var2; + ROLLBACK TO s1; + -- force cleaning by touching another session variable + SELECT var2; +COMMIT; +-- should be ok +SELECT var1; + +-- repeated aborted transaction +BEGIN; DROP VARIABLE var1; ROLLBACK; +BEGIN; DROP VARIABLE var1; ROLLBACK; +BEGIN; DROP VARIABLE var1; ROLLBACK; + +-- should be ok +SELECT var1; + +DROP VARIABLE var1, var2; -- 2.48.1