From e025f98d806052f581f50035f4556ad571abed15 Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Sun, 28 Jan 2024 20:58:23 +0100 Subject: [PATCH 05/19] memory cleaning after DROP VARIABLE Accept of sinval message invalidates entries in sessionvars hash table. These entries are validated before any read or write operations on session variables. When the entry cannot be validated, then it is removed. The removing can be delayed when variable is dropped inside current transaction. This transaction can be aborted and in this case we want to preserve content of the variable. --- src/backend/access/transam/xact.c | 4 + src/backend/catalog/pg_variable.c | 4 + src/backend/commands/session_variable.c | 177 +++++++++++++- src/include/commands/session_variable.h | 3 + .../isolation/expected/session-variable.out | 109 +++++++++ src/test/isolation/isolation_schedule | 1 + .../isolation/specs/session-variable.spec | 50 ++++ .../regress/expected/session_variables.out | 219 ++++++++++++++++++ src/test/regress/sql/session_variables.sql | 121 ++++++++++ 9 files changed, 682 insertions(+), 6 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/access/transam/xact.c b/src/backend/access/transam/xact.c index 9bda1aa6bc..a8f5efad5d 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -36,6 +36,7 @@ #include "catalog/pg_enum.h" #include "catalog/storage.h" #include "commands/async.h" +#include "commands/session_variable.h" #include "commands/tablecmds.h" #include "commands/trigger.h" #include "common/pg_prng.h" @@ -2267,6 +2268,9 @@ CommitTransaction(void) */ smgrDoPendingSyncs(true, is_parallel_worker); + /* Remove values of dropped session variables from memory */ + AtPreEOXact_SessionVariables(); + /* close large objects before lower-level cleanup */ AtEOXact_LargeObject(true); diff --git a/src/backend/catalog/pg_variable.c b/src/backend/catalog/pg_variable.c index 1c73181c3d..0b21204bc4 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" @@ -254,4 +255,7 @@ DropVariableById(Oid varid) ReleaseSysCache(tup); table_close(rel, RowExclusiveLock); + + /* Do the necessary cleanup if needed in local memory */ + SessionVariableDropPostprocess(varid); } diff --git a/src/backend/commands/session_variable.c b/src/backend/commands/session_variable.c index c796858712..d9169820a7 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" @@ -75,6 +76,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 + * variable dropped by the local backend that may be eventually + * rollbacked. + */ + LocalTransactionId drop_lxid; + /* * Stored value and type description can be outdated when we receive * sinval message. We have to check always if the stored data are @@ -91,6 +100,19 @@ static HTAB *sessionvars = NULL; /* hash table for session variables */ static MemoryContext SVariableMemoryContext = NULL; +/* true after accepted sinval message */ +static bool needs_validation = false; + +/* + * The content of session variables is not removed immediately. When it + * is possible we do this at the transaction end. But when the transaction failed, + * we cannot do it, because we lost access to the system catalog. So we + * try to do it in the next transaction before any get or set of any session + * variable. We don't want to repeat this opening cleaning in transaction, + * So we store the id of the transaction where opening validation was done. + */ +static LocalTransactionId validated_lxid = InvalidLocalTransactionId; + /* * Callback function for session variable invalidation. */ @@ -123,6 +145,40 @@ 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 + * don't automatically remove the local variable storage in + * validate_all_session_variables, as the DROP VARIABLE will send + * an invalidation message. + */ + svar->is_valid = false; + svar->drop_lxid = MyProc->vxid.lxid; + + needs_validation = true; } } } @@ -176,6 +232,83 @@ is_session_variable_valid(SVariable svar) return result; } +/* + * It checks all possibly invalid entries against the system catalog. + * During this validation, the system cache can be invalidated, and the + * some sinval message can be accepted. This routine doesn't ensure + * all living entries of sessionvars will have is_valid flag, but it ensures + * that all entries are checked once. + * + * This routine is called before any usage (read, write, debug) of session + * variables (only once in a transaction) or at a transaction end. At the + * end of transaction (specified by true of argument atEOX) we can + * throw all invalid variables. Inside the transaction (atEOX is false) we want + * to postpone cleaning of variables dropped inside the current transaction. + * The current transaction can be aborted, the related drop command will be + * aborted too. In this case we don't want lose the content of the variable, + * and therefore we need to hold the content of the dropped session variable + * until the end of the transaction (where the variable was dropped). + */ +static void +remove_invalid_session_variables(bool atEOX) +{ + HASH_SEQ_STATUS status; + SVariable svar; + + /* + * The validation requires an access to system catalog, and then the + * session state should be "in transaction". + */ + Assert(IsTransactionState()); + + if (!needs_validation || !sessionvars) + return; + + /* + * Reset this flag here, before we start the validation. It can be set to + * on by incomming sinval message. + */ + 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 (!atEOX && svar->drop_lxid == MyProc->vxid.lxid) + { + 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; + } + } +} + +/* + * Remove values of dropped session variables from memory. + */ +void +AtPreEOXact_SessionVariables(void) +{ + /* We cannot to do it when transaction is aborted */ + Assert(IsTransactionState()); + + remove_invalid_session_variables(true); +} + /* * Update attributes cached in svar */ @@ -205,6 +338,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; @@ -335,26 +470,44 @@ get_session_variable(Oid varid) if (!sessionvars) create_sessionvars_hashtables(); + if (validated_lxid == InvalidLocalTransactionId || + validated_lxid != MyProc->vxid.lxid) + { + /* Throw invalid entries, skip entries dropped by this transaction. */ + remove_invalid_session_variables(false); + + /* Don't repeat it in this transaction */ + validated_lxid = MyProc->vxid.lxid; + } + svar = (SVariable) hash_search(sessionvars, &varid, HASH_ENTER, &found); if (found) { + /* + * The session variable can be dropped by DROP VARIABLE command, + * but effect of this command can be reverted by ROLLBACK to savepoint, + * so we can work here with readable value in variable marked as invalid. + */ if (!svar->is_valid) { /* * The variable can be flagged as invalid by processing invalidation * message, but can be validated by recheck against system catalog. + * + * If we access this session variable, then the variable should be + * possibly validated. The oid should be valid, because related + * session variable is locked already, and remove_invalid_session_variables + * should to remove variables dropped by other transactions. */ if (is_session_variable_valid(svar)) svar->is_valid = true; else - /* - * When the value cannot be validated, we can safely throw it. - * There is oid in catalog, but it is related to different - * session variable (different create_lsn). - */ - free_session_variable_value(svar); + { + /* We don't expect it */ + elog(ERROR, "unexpected state of session variable %u", varid); + } } } else @@ -415,6 +568,16 @@ SetSessionVariable(Oid varid, Datum value, bool isNull) if (!sessionvars) create_sessionvars_hashtables(); + if (validated_lxid == InvalidLocalTransactionId || + validated_lxid != MyProc->vxid.lxid) + { + /* Throw invalid entries, skip entries dropped by this transaction. */ + remove_invalid_session_variables(false); + + /* Don't repeat it in this transaction */ + validated_lxid = MyProc->vxid.lxid; + } + svar = (SVariable) hash_search(sessionvars, &varid, HASH_ENTER, &found); @@ -622,6 +785,8 @@ pg_session_variables(PG_FUNCTION_ARGS) elog(DEBUG1, "pg_session_variables start"); + remove_invalid_session_variables(false); + InitMaterializedSRF(fcinfo, 0); if (sessionvars) diff --git a/src/include/commands/session_variable.h b/src/include/commands/session_variable.h index 443afbafd4..0d4c635ae7 100644 --- a/src/include/commands/session_variable.h +++ b/src/include/commands/session_variable.h @@ -21,6 +21,9 @@ #include "tcop/cmdtag.h" #include "utils/queryenvironment.h" +extern void SessionVariableDropPostprocess(Oid varid); +extern void AtPreEOXact_SessionVariables(void); + extern void SetSessionVariable(Oid varid, Datum value, bool isNull); extern void SetSessionVariableWithSecurityCheck(Oid varid, Datum value, bool isNull); extern Datum GetSessionVariable(Oid varid, bool *isNull, Oid *typid); diff --git a/src/test/isolation/expected/session-variable.out b/src/test/isolation/expected/session-variable.out new file mode 100644 index 0000000000..2968cce089 --- /dev/null +++ b/src/test/isolation/expected/session-variable.out @@ -0,0 +1,109 @@ +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 +------+----+------- +(0 rows) + +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 0342eb39e4..de72bb1b9e 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -114,3 +114,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 0000000000..c864fee400 --- /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 637402e52c..3748e1439b 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1004,3 +1004,222 @@ SELECT count(*) FROM pg_session_variables(); ------- 0 (1 row) + +-- dropped variables should be removed from memory +-- at the end of transaction or before next usage +-- of any session variable in 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(); + count +------- + 0 +(1 row) + +-- the content of value should be preserved when variable is dropped +-- by 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(); + 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(); + 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(); + 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 3f0c8ef4e8..4643cf138a 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -699,3 +699,124 @@ DISCARD VARIABLES; -- should be zero again SELECT count(*) FROM pg_session_variables(); + +-- dropped variables should be removed from memory +-- at the end of transaction or before next usage +-- of any session variable in 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(); + +-- the content of value should be preserved when variable is dropped +-- by 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(); + +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(); + +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(); + +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.45.2