Hi,
On 2022-11-28 16:33:20 -0500, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > Something like the attached. Still needs a bit of polish, e.g. adding the
> > test
> > case from above.
>
> > I'm a bit uncomfortable adding a function call below
> > * Perform swapping of the relcache entry contents. Within this
> > * process the old entry is momentarily invalid, so there
> > *must* be no
> > * possibility of CHECK_FOR_INTERRUPTS within this sequence. Do
> > it in
> > * all-in-line code for safety.
>
> Ugh. I don't know what pgstat_unlink_relation does, but assuming
> that it can never throw an error seems like a pretty bad idea,
I don't think it'd be an issue - it just resets the pointer from a pgstat
entry to the relcache entry.
But you're right:
> Can't that part be done outside the critical section?
we can do that. See the attached.
Do we have any cases of relcache entries changing their relkind?
Greetings,
Andres Freund
diff --git i/src/backend/utils/cache/relcache.c w/src/backend/utils/cache/relcache.c
index bd6cd4e47b5..e2521b51ad4 100644
--- i/src/backend/utils/cache/relcache.c
+++ w/src/backend/utils/cache/relcache.c
@@ -2626,6 +2626,7 @@ RelationClearRelation(Relation relation, bool rebuild)
bool keep_rules;
bool keep_policies;
bool keep_partkey;
+ bool keep_pgstats;
/* Build temporary entry, but don't link it into hashtable */
newrel = RelationBuildDesc(save_relid, false);
@@ -2667,6 +2668,21 @@ RelationClearRelation(Relation relation, bool rebuild)
/* partkey is immutable once set up, so we can always keep it */
keep_partkey = (relation->rd_partkey != NULL);
+ /*
+ * Keep stats pointers, except when the relkind changes
+ * (e.g. converting tables into views). Different kinds of relations
+ * might have different types of stats.
+ *
+ * If we don't want to keep the stats, unlink the stats and relcache
+ * entry (and do so before entering the "critical section"
+ * below). This is important because otherwise
+ * PgStat_TableStatus->relation would get out of sync with
+ * relation->pgstat_info.
+ */
+ keep_pgstats = relation->rd_rel->relkind == newrel->rd_rel->relkind;
+ if (!keep_pgstats)
+ pgstat_unlink_relation(relation);
+
/*
* Perform swapping of the relcache entry contents. Within this
* process the old entry is momentarily invalid, so there *must* be no
@@ -2720,9 +2736,14 @@ RelationClearRelation(Relation relation, bool rebuild)
SWAPFIELD(RowSecurityDesc *, rd_rsdesc);
/* toast OID override must be preserved */
SWAPFIELD(Oid, rd_toastoid);
+
/* pgstat_info / enabled must be preserved */
- SWAPFIELD(struct PgStat_TableStatus *, pgstat_info);
- SWAPFIELD(bool, pgstat_enabled);
+ if (keep_pgstats)
+ {
+ SWAPFIELD(struct PgStat_TableStatus *, pgstat_info);
+ SWAPFIELD(bool, pgstat_enabled);
+ }
+
/* preserve old partition key if we have one */
if (keep_partkey)
{
diff --git i/src/test/regress/expected/create_view.out w/src/test/regress/expected/create_view.out
index 17ca29ddbf7..4c108c4c40b 100644
--- i/src/test/regress/expected/create_view.out
+++ w/src/test/regress/expected/create_view.out
@@ -2202,6 +2202,29 @@ select pg_get_viewdef('tt26v', true);
FROM ( VALUES (1,2,3)) v(x, y, z);
(1 row)
+-- Test that changing the relkind of a relcache entry doesn't cause
+-- trouble. Prior instances of where it did:
+-- caldanm2yxz+zotv7y5zbd5wkt8o0ld3yxikuu3dcycvxf7g...@mail.gmail.com
+-- caldanm3oza-8wbps2jd1g5_gjrr-x3ywrjpek-mf5asrrvz...@mail.gmail.com
+CREATE TABLE tt26(c int);
+BEGIN;
+CREATE TABLE tt27(c int);
+SAVEPOINT q;
+CREATE RULE "_RETURN" AS ON SELECT TO tt27 DO INSTEAD SELECT * FROM tt26;
+SELECT * FROM tt27;
+ c
+---
+(0 rows)
+
+ROLLBACK TO q;
+CREATE RULE "_RETURN" AS ON SELECT TO tt27 DO INSTEAD SELECT * FROM tt26;
+ROLLBACK;
+BEGIN;
+CREATE TABLE tt28(c int);
+CREATE RULE "_RETURN" AS ON SELECT TO tt28 DO INSTEAD SELECT * FROM tt26;
+CREATE RULE "_RETURN" AS ON SELECT TO tt28 DO INSTEAD SELECT * FROM tt26;
+ERROR: "tt28" is already a view
+ROLLBACK;
-- clean up all the random objects we made above
DROP SCHEMA temp_view_test CASCADE;
NOTICE: drop cascades to 27 other objects
@@ -2233,7 +2256,7 @@ drop cascades to view aliased_view_2
drop cascades to view aliased_view_3
drop cascades to view aliased_view_4
DROP SCHEMA testviewschm2 CASCADE;
-NOTICE: drop cascades to 77 other objects
+NOTICE: drop cascades to 78 other objects
DETAIL: drop cascades to table t1
drop cascades to view temporal1
drop cascades to view temporal2
@@ -2311,3 +2334,4 @@ drop cascades to view tt23v
drop cascades to view tt24v
drop cascades to view tt25v
drop cascades to view tt26v
+drop cascades to table tt26
diff --git i/src/test/regress/sql/create_view.sql w/src/test/regress/sql/create_view.sql
index 8838a40f7ab..f305632c6aa 100644
--- i/src/test/regress/sql/create_view.sql
+++ w/src/test/regress/sql/create_view.sql
@@ -813,6 +813,29 @@ select x + y + z as c1,
from (values(1,2,3)) v(x,y,z);
select pg_get_viewdef('tt26v', true);
+
+-- Test that changing the relkind of a relcache entry doesn't cause
+-- trouble. Prior instances of where it did:
+-- caldanm2yxz+zotv7y5zbd5wkt8o0ld3yxikuu3dcycvxf7g...@mail.gmail.com
+-- caldanm3oza-8wbps2jd1g5_gjrr-x3ywrjpek-mf5asrrvz...@mail.gmail.com
+CREATE TABLE tt26(c int);
+
+BEGIN;
+CREATE TABLE tt27(c int);
+SAVEPOINT q;
+CREATE RULE "_RETURN" AS ON SELECT TO tt27 DO INSTEAD SELECT * FROM tt26;
+SELECT * FROM tt27;
+ROLLBACK TO q;
+CREATE RULE "_RETURN" AS ON SELECT TO tt27 DO INSTEAD SELECT * FROM tt26;
+ROLLBACK;
+
+BEGIN;
+CREATE TABLE tt28(c int);
+CREATE RULE "_RETURN" AS ON SELECT TO tt28 DO INSTEAD SELECT * FROM tt26;
+CREATE RULE "_RETURN" AS ON SELECT TO tt28 DO INSTEAD SELECT * FROM tt26;
+ROLLBACK;
+
+
-- clean up all the random objects we made above
DROP SCHEMA temp_view_test CASCADE;
DROP SCHEMA testviewschm2 CASCADE;
