Hi Marko,
Marko Grujic <[email protected]> wrote:
> The reason it wasn't re-created is because the internal static variable
> (myTempNamespace) is left holding the 16409 value internally.
This came up in 2019-2020 [1]. a052f6cbb84 forbade DROP SCHEMA on temp
schemas and was reverted (65192e02441) after Robert and Tom argued that
a superuser should be able to do it, and that code should cope with a
missing schema instead of crashing; 80d76be51cf then fixed autovacuum
that way. Michael pointed out the stale myTempNamespace in that thread,
but it was left as is, so your patches fill that gap in the direction
agreed there. (Cc'ing Michael for that reason.)
I tested them on master d39fda1cc40, with --enable-cassert, and master
without them as the control. Both reported problems are fixed, and 0002
covers more than tables. After dropping the own temp schema, on master
CREATE DOMAIN, CREATE FUNCTION and CREATE STATISTICS in pg_temp fail
with "referenced schema was concurrently dropped", while a composite
type and a temp table with a serial column and an index are created in
the missing schema: 9 rows in pg_class and pg_type point to it. With
0002, everything is created and nothing is left dangling. The same when
another superuser session drops
the schema: on master that session leaves a pg_class row with a missing
schema behind when it exits; with 0002, it doesn't.
About 0001: the strlist_to_textarray() part is worth a line in the
commit message, because on master a NULL name is silently dropped from
the array, and that is worse than it looks. With a table whose
pg_namespace row is gone (I deleted it with allow_system_table_mods) and
another table of the same name in public,
pg_get_object_address('table',
(pg_identify_object_as_address('pg_class'::regclass, oid,
0)).object_names,
'{}')
resolves to public.x on master. With 0001 the names are {NULL,x} and it
fails with "name or argument lists may not contain nulls". On master,
pg_identify_object() on that table crashes too, as in your report.
One problem with 0002: if the DROP SCHEMA is rolled back, the session
loses its temp schema, and the next temp table empties it.
CREATE TEMP TABLE keep(a int);
INSERT INTO keep VALUES (1), (2), (3);
BEGIN;
DROP SCHEMA pg_temp_0 CASCADE;
CREATE TEMP TABLE t3(a int);
ROLLBACK;
SELECT count(*) FROM keep;
ERROR: relation "keep" does not exist
keep is still in pg_class, in pg_temp_0, but pg_my_temp_schema() is 0,
pg_temp is gone from the search path, and "SELECT * FROM pg_temp_0.keep"
fails with "cannot access temporary tables of other sessions". The next
CREATE TEMP TABLE goes through InitTempTableNamespace(), finds pg_temp_0,
takes it for a crashed backend's leftover, and RemoveTempRelations()
drops keep. It's the same with ROLLBACK TO SAVEPOINT. On master, keep
still has its 3 rows in both cases.
Nothing undoes the reset in AccessTempTableNamespace() on abort:
AtEOXact_Namespace() sees the myTempNamespaceSubID set by the new
InitTempTableNamespace() and forgets the new schema, but nothing brings
back the old one. The attached diff, on top of 0002, remembers the
forgotten schema and the subtransaction that forgot it, and puts it back
if that subtransaction aborts, unless the schema was created in it; on
subtransaction commit it moves to the parent, like myTempNamespaceSubID.
If the drop was committed earlier, what it puts back is still stale, and
the next access forgets it again. The diff adds both rollback cases to
your test in temp.sql, which fails with 0002 alone.
With it, those two cases behave as on master, and so do these:
- a temp schema created, dropped and rolled back in one transaction
- a committed drop, then an aborted CREATE TEMP TABLE, then another
- a drop undone by ROLLBACK TO an outer savepoint, with a savepoint
released in between
- a drop released from a savepoint, then the whole transaction rolled
back
0002 alone loses keep in the last two. make check (239) and isolation
(133) pass with 0001 and 0002, and with the diff on top; pgindent leaves
it unchanged.
The attached temp_schema_drop_cases.sql has all of the above as a plain
psql script, one case per session, with what each should return. Case 8
deletes a pg_namespace row, so it is meant for a scratch database. On
master cases 1, 5 and 8 fail (8 by crashing, as the last statement), with
0001 and 0002 cases 2, 3, 6 and 7, and with the diff none.
[1]
https://www.postgresql.org/message-id/CAKYtNAr9Zq%3D1-ww4etHo-VCC-k120YxZy5OS01VkaLPaDbv2tg%40mail.gmail.com
Regards,
Manu
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index f2480509e80..9d2053a9d50 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -207,6 +207,17 @@ static SubTransactionId myTempNamespaceSubID =
InvalidSubTransactionId;
/* Have we registered RemoveTempRelationsCallback for this session yet? */
static bool myTempNamespaceCleanupRegistered = false;
+/*
+ * The temp namespace we forgot because it seemed to be gone, and the
+ * subtransaction in which we did that. If that subtransaction aborts, the
+ * DROP SCHEMA that made it disappear may be rolled back with it, so we go
+ * back to the namespace we had.
+ */
+static Oid forgottenTempNamespace = InvalidOid;
+static Oid forgottenTempToastNamespace = InvalidOid;
+static SubTransactionId forgottenTempNamespaceCreateSubID =
InvalidSubTransactionId;
+static SubTransactionId forgottenTempNamespaceSubID = InvalidSubTransactionId;
+
/*
* This is the user's textual search path specification --- it's the value
* of the GUC variable 'search_path'.
@@ -4451,6 +4462,14 @@ AccessTempTableNamespace(bool force)
!SearchSysCacheExists1(NAMESPACEOID,
ObjectIdGetDatum(myTempNamespace)))
{
+ if (forgottenTempNamespaceSubID == InvalidSubTransactionId)
+ {
+ forgottenTempNamespace = myTempNamespace;
+ forgottenTempToastNamespace = myTempToastNamespace;
+ forgottenTempNamespaceCreateSubID =
myTempNamespaceSubID;
+ forgottenTempNamespaceSubID =
GetCurrentSubTransactionId();
+ }
+
myTempNamespace = InvalidOid;
myTempToastNamespace = InvalidOid;
myTempNamespaceSubID = InvalidSubTransactionId;
@@ -4600,6 +4619,32 @@ InitTempTableNamespace(void)
searchPathCacheValid = false;
}
+/*
+ * Go back to the temp namespace that AccessTempTableNamespace() forgot,
+ * because the (sub)transaction that forgot it, abortingSubid, is aborting.
+ * If that namespace was created within abortingSubid, its creation is being
+ * rolled back too, and there is nothing to go back to.
+ */
+static void
+RestoreForgottenTempNamespace(SubTransactionId abortingSubid)
+{
+ if (forgottenTempNamespaceCreateSubID == InvalidSubTransactionId ||
+ forgottenTempNamespaceCreateSubID < abortingSubid)
+ {
+ myTempNamespace = forgottenTempNamespace;
+ myTempToastNamespace = forgottenTempToastNamespace;
+ myTempNamespaceSubID = forgottenTempNamespaceCreateSubID;
+ MyProc->tempNamespaceId = forgottenTempNamespace;
+ baseSearchPathValid = false; /* need to rebuild list */
+ searchPathCacheValid = false;
+ }
+
+ forgottenTempNamespace = InvalidOid;
+ forgottenTempToastNamespace = InvalidOid;
+ forgottenTempNamespaceCreateSubID = InvalidSubTransactionId;
+ forgottenTempNamespaceSubID = InvalidSubTransactionId;
+}
+
/*
* End-of-transaction cleanup for namespaces.
*/
@@ -4646,6 +4691,18 @@ AtEOXact_Namespace(bool isCommit, bool parallel)
myTempNamespaceSubID = InvalidSubTransactionId;
}
+ if (forgottenTempNamespaceSubID != InvalidSubTransactionId && !parallel)
+ {
+ if (isCommit)
+ {
+ forgottenTempNamespace = InvalidOid;
+ forgottenTempToastNamespace = InvalidOid;
+ forgottenTempNamespaceCreateSubID =
InvalidSubTransactionId;
+ forgottenTempNamespaceSubID = InvalidSubTransactionId;
+ }
+ else
+ RestoreForgottenTempNamespace(TopSubTransactionId);
+ }
}
/*
@@ -4686,6 +4743,18 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId
mySubid,
MyProc->tempNamespaceId = InvalidOid;
}
}
+
+ if (forgottenTempNamespaceSubID == mySubid)
+ {
+ if (isCommit)
+ {
+ forgottenTempNamespaceSubID = parentSubid;
+ if (forgottenTempNamespaceCreateSubID == mySubid)
+ forgottenTempNamespaceCreateSubID = parentSubid;
+ }
+ else
+ RestoreForgottenTempNamespace(mySubid);
+ }
}
/*
diff --git a/src/test/regress/expected/temp.out
b/src/test/regress/expected/temp.out
index 476e312e352..92b61c09e25 100644
--- a/src/test/regress/expected/temp.out
+++ b/src/test/regress/expected/temp.out
@@ -601,3 +601,38 @@ SELECT count(*) AS orphans FROM pg_class c
0
(1 row)
+-- if the drop is rolled back, the session keeps its temp schema and tables
+INSERT INTO test_temp_t3 VALUES (1), (2), (3);
+SELECT pg_my_temp_schema()::regnamespace AS mytempschema \gset
+BEGIN;
+DROP SCHEMA :mytempschema CASCADE;
+NOTICE: drop cascades to table test_temp_t3
+CREATE TEMP TABLE test_temp_t4(f1 int);
+ROLLBACK;
+SELECT count(*) FROM test_temp_t3;
+ count
+-------
+ 3
+(1 row)
+
+BEGIN;
+SAVEPOINT sp;
+DROP SCHEMA :mytempschema CASCADE;
+NOTICE: drop cascades to table test_temp_t3
+CREATE TEMP TABLE test_temp_t4(f1 int);
+ROLLBACK TO sp;
+CREATE TEMP TABLE test_temp_t5(f1 int);
+COMMIT;
+SELECT count(*) FROM test_temp_t3;
+ count
+-------
+ 3
+(1 row)
+
+SELECT pg_my_temp_schema()::regnamespace = :'mytempschema'::regnamespace
+ AS same_temp_schema;
+ same_temp_schema
+------------------
+ t
+(1 row)
+
diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql
index 7d7e18ce8c8..95e41865242 100644
--- a/src/test/regress/sql/temp.sql
+++ b/src/test/regress/sql/temp.sql
@@ -439,3 +439,22 @@ SELECT EXISTS (SELECT 1 FROM pg_namespace WHERE oid =
pg_my_temp_schema())
AS temp_schema_exists;
SELECT count(*) AS orphans FROM pg_class c
WHERE NOT EXISTS (SELECT 1 FROM pg_namespace n WHERE n.oid =
c.relnamespace);
+
+-- if the drop is rolled back, the session keeps its temp schema and tables
+INSERT INTO test_temp_t3 VALUES (1), (2), (3);
+SELECT pg_my_temp_schema()::regnamespace AS mytempschema \gset
+BEGIN;
+DROP SCHEMA :mytempschema CASCADE;
+CREATE TEMP TABLE test_temp_t4(f1 int);
+ROLLBACK;
+SELECT count(*) FROM test_temp_t3;
+BEGIN;
+SAVEPOINT sp;
+DROP SCHEMA :mytempschema CASCADE;
+CREATE TEMP TABLE test_temp_t4(f1 int);
+ROLLBACK TO sp;
+CREATE TEMP TABLE test_temp_t5(f1 int);
+COMMIT;
+SELECT count(*) FROM test_temp_t3;
+SELECT pg_my_temp_schema()::regnamespace = :'mytempschema'::regnamespace
+ AS same_temp_schema;
-- Cases around DROP SCHEMA of the session's own pg_temp_N.
-- Run as a superuser in a scratch database: psql -X -f
temp_schema_drop_cases.sql
-- Each case starts a new session (\c -). "expected" is master's behavior,
-- or what should happen where master is broken.
\set VERBOSITY terse
\pset footer off
\echo
\echo == 1. drop committed, then other temp objects
\echo expected: all created, nothing pointing to a missing schema
\echo (master: errors, and rows left in pg_class/pg_type)
\c -
CREATE TEMP TABLE z(a int);
SELECT pg_my_temp_schema()::regnamespace AS s \gset
DROP SCHEMA :s CASCADE;
CREATE TYPE pg_temp.ty AS (a int);
CREATE DOMAIN pg_temp.dom AS int;
CREATE FUNCTION pg_temp.eq3(int, int) RETURNS bool LANGUAGE sql AS 'SELECT $1 =
$2';
CREATE OPERATOR pg_temp.=== (LEFTARG = int, RIGHTARG = int, FUNCTION =
pg_temp.eq3);
CREATE TEMP TABLE ser(id serial PRIMARY KEY, b int);
CREATE INDEX ON ser(b);
CREATE STATISTICS pg_temp.st ON id, b FROM ser;
SELECT EXISTS (SELECT 1 FROM pg_namespace WHERE oid = pg_my_temp_schema()) AS
temp_schema_exists,
(SELECT count(*) FROM pg_class c WHERE NOT EXISTS (SELECT 1 FROM
pg_namespace n WHERE n.oid = c.relnamespace)) AS dangling_class,
(SELECT count(*) FROM pg_type t WHERE NOT EXISTS (SELECT 1 FROM
pg_namespace n WHERE n.oid = t.typnamespace)) AS dangling_type;
\echo
\echo == 2. drop rolled back
\echo expected: keep has 3 rows, before and after the next CREATE TEMP TABLE
\c -
CREATE TEMP TABLE keep(a int);
INSERT INTO keep VALUES (1), (2), (3);
SELECT pg_my_temp_schema()::regnamespace AS s \gset
BEGIN;
DROP SCHEMA :s CASCADE;
CREATE TEMP TABLE t(a int);
ROLLBACK;
SELECT count(*) AS keep_rows FROM keep;
CREATE TEMP TABLE t2(a int);
SELECT count(*) AS keep_rows FROM keep;
\echo
\echo == 3. drop undone by ROLLBACK TO SAVEPOINT
\echo expected: keep has 3 rows, before and after the next CREATE TEMP TABLE
\c -
CREATE TEMP TABLE keep(a int);
INSERT INTO keep VALUES (1), (2), (3);
SELECT pg_my_temp_schema()::regnamespace AS s \gset
BEGIN;
SAVEPOINT sp;
DROP SCHEMA :s CASCADE;
CREATE TEMP TABLE t(a int);
ROLLBACK TO sp;
COMMIT;
SELECT count(*) AS keep_rows FROM keep;
CREATE TEMP TABLE t2(a int);
SELECT count(*) AS keep_rows FROM keep;
\echo
\echo == 4. temp schema created, dropped and rolled back in one transaction
\echo expected: c has 1 row, temp schema exists
\c -
BEGIN;
CREATE TEMP TABLE a(x int);
SELECT pg_my_temp_schema()::regnamespace AS s \gset
DROP SCHEMA :s CASCADE;
CREATE TEMP TABLE b(x int);
ROLLBACK;
CREATE TEMP TABLE c(x int);
INSERT INTO c VALUES (1);
SELECT count(*) AS c_rows,
EXISTS (SELECT 1 FROM pg_namespace WHERE oid = pg_my_temp_schema()) AS
temp_schema_exists;
\echo
\echo == 5. drop committed, then an aborted CREATE TEMP TABLE, then another
\echo expected: c has 1 row, temp schema exists (master: it doesn't)
\c -
CREATE TEMP TABLE a(x int);
SELECT pg_my_temp_schema()::regnamespace AS s \gset
DROP SCHEMA :s CASCADE;
BEGIN;
CREATE TEMP TABLE b(x int);
ROLLBACK;
CREATE TEMP TABLE c(x int);
INSERT INTO c VALUES (1);
SELECT count(*) AS c_rows,
EXISTS (SELECT 1 FROM pg_namespace WHERE oid = pg_my_temp_schema()) AS
temp_schema_exists;
\echo
\echo == 6. drop undone by ROLLBACK TO an outer savepoint, a savepoint released
in between
\echo expected: keep has 3 rows
\c -
CREATE TEMP TABLE keep(x int);
INSERT INTO keep VALUES (1), (2), (3);
SELECT pg_my_temp_schema()::regnamespace AS s \gset
BEGIN;
SAVEPOINT a;
SAVEPOINT b;
DROP SCHEMA :s CASCADE;
SAVEPOINT c;
CREATE TEMP TABLE t(x int);
RELEASE c;
ROLLBACK TO b;
CREATE TEMP TABLE t2(x int);
COMMIT;
SELECT count(*) AS keep_rows FROM keep;
\echo
\echo == 7. drop released from a savepoint, then the transaction rolled back
\echo expected: keep has 3 rows, before and after the next CREATE TEMP TABLE
\c -
CREATE TEMP TABLE keep(x int);
INSERT INTO keep VALUES (1), (2), (3);
SELECT pg_my_temp_schema()::regnamespace AS s \gset
BEGIN;
SAVEPOINT a;
DROP SCHEMA :s CASCADE;
CREATE TEMP TABLE t(x int);
RELEASE a;
ROLLBACK;
SELECT count(*) AS keep_rows FROM keep;
CREATE TEMP TABLE t3(x int);
SELECT count(*) AS keep_rows FROM keep;
\echo
\echo == 8. a table whose pg_namespace row is gone, and a same-named table in
public
\echo expected: no crash; the round trip must not land on public.x
\echo (master: pg_identify_object crashes, and the round trip returns
public.x)
\echo This one deletes a catalog row: run it in a database you can throw
away.
\c -
CREATE SCHEMA gone;
CREATE TABLE gone.x(a int);
CREATE TABLE public.x(b text);
SELECT oid AS xoid FROM pg_class WHERE relnamespace = 'gone'::regnamespace AND
relname = 'x' \gset
SET allow_system_table_mods = on;
DELETE FROM pg_namespace WHERE nspname = 'gone';
RESET allow_system_table_mods;
SELECT :xoid AS gone_x_oid, 'public.x'::regclass::oid AS public_x_oid;
SELECT (pg_identify_object_as_address('pg_class'::regclass, :xoid,
0)).object_names;
SELECT objid AS round_trip_oid
FROM pg_get_object_address('table',
(pg_identify_object_as_address('pg_class'::regclass, :xoid,
0)).object_names, '{}');
SELECT pg_identify_object('pg_class'::regclass, :xoid, 0);
