From 4cb7f40ad8f2fef80de45c710cc7dc9e26d25572 Mon Sep 17 00:00:00 2001 From: "okbob@github.com" Date: Fri, 16 Jun 2023 06:40:58 +0200 Subject: [PATCH 4/9] 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 | 170 +++++++++++++- 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 | 218 ++++++++++++++++++ src/test/regress/sql/session_variables.sql | 120 ++++++++++ 9 files changed, 673 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 ba3f6114e4..ab646aa02b 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" @@ -2224,6 +2225,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 10b6187bbe..d962108671 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 @@ DropVariable(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 12b02ca1e0..571e62c569 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 + * 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 @@ -83,6 +92,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. * @@ -125,6 +147,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->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->lxid; + + needs_validation = true; } } } @@ -182,6 +238,81 @@ is_session_variable_valid(SVariable svar) return result; } +/* + * Does check 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 + * so all entries are checked once. + * + * This routine can be called somewhere inside transaction or at an transaction + * end. When atEOX argument is false, then we are inside transaction, and we + * don't want to throw entries related to session variables dropped in current + * transaction. + */ +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) + return; + + /* + * Reset, this flag here, before we start the validation. It can be set to + * on by incomming sinval message. + */ + needs_validation = false; + + if (!sessionvars) + return; + + 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->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); +} + /* * When the stored content is not consistent with catalog, release it. */ @@ -221,6 +352,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->freeval = false; svar->value = (Datum) 0; @@ -350,6 +483,16 @@ get_session_variable(Oid varid) if (!sessionvars) create_sessionvars_hashtables(); + if (validated_lxid == InvalidLocalTransactionId || + validated_lxid != MyProc->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->lxid; + } + svar = (SVariable) hash_search(sessionvars, &varid, HASH_ENTER, &found); @@ -360,10 +503,18 @@ get_session_variable(Oid varid) if (is_session_variable_valid(svar)) svar->is_valid = true; else + + /* + * In this case, the session variable was dropped, but we + * hadn't possibility to remove Do it now. + */ invalidate_session_variable(svar); } } else + svar->is_valid = false; + + if (!svar->is_valid) { setup_session_variable(svar, varid); @@ -373,12 +524,6 @@ get_session_variable(Oid varid) varid); } - if (!svar->is_valid) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("the stored value of session variable (oid:%u) is not valid", - varid))); - /* Ensure so returned data is still correct domain */ if (svar->is_domain) { @@ -420,6 +565,16 @@ SetSessionVariable(Oid varid, Datum value, bool isNull) if (!sessionvars) create_sessionvars_hashtables(); + if (validated_lxid == InvalidLocalTransactionId || + validated_lxid != MyProc->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->lxid; + } + svar = (SVariable) hash_search(sessionvars, &varid, HASH_ENTER, &found); @@ -607,6 +762,7 @@ ExecuteLetStmt(ParseState *pstate, PopActiveSnapshot(); } + /* * Fast drop of the complete content of all session variables hash table, and * cleanup of any list that wouldn't be relevant anymore. @@ -642,6 +798,8 @@ pg_session_variables(PG_FUNCTION_ARGS) elog(DEBUG1, "pg_session_variables start"); + remove_invalid_session_variables(true); + InitMaterializedSRF(fcinfo, 0); if (sessionvars) diff --git a/src/include/commands/session_variable.h b/src/include/commands/session_variable.h index e4fab99439..ad5aa73dc6 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 4fc56ae99c..809f47b941 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -110,3 +110,4 @@ test: serializable-parallel test: serializable-parallel-2 test: serializable-parallel-3 test: matview-write-skew +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 c0f624bf55..719cf71a7e 100644 --- a/src/test/regress/expected/session_variables.out +++ b/src/test/regress/expected/session_variables.out @@ -1018,3 +1018,221 @@ 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 f6f0336646..2e5a5bf888 100644 --- a/src/test/regress/sql/session_variables.sql +++ b/src/test/regress/sql/session_variables.sql @@ -683,3 +683,123 @@ 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.41.0