Hi, On 2022-11-28 10:50:13 -0800, Andres Freund wrote: > On 2022-11-28 13:37:16 -0500, Tom Lane wrote: > > Uh-huh. I've not bothered to trace this in detail, but presumably > > what is happening is that the first CREATE RULE converts the table > > to a view, and then the ROLLBACK undoes that so far as the catalogs > > are concerned, but probably doesn't undo related pg_stats state > > changes fully. Then we're in a bad state that will cause problems. > > (It still crashes if you replace the second CREATE RULE with > > "select * from t".) > > Yea. I haven't yet fully traced through this, but presumably relcache inval > doesn't fix this because we don't want to loose pending stats after DDL. > > Perhaps we need to add a rule about not swapping pgstat* in > RelationClearRelation() when relkind changes?
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. but it's not the first, see MemoryContextSetParent(). Greetings, Andres Freund
diff --git i/src/backend/utils/activity/pgstat_relation.c w/src/backend/utils/activity/pgstat_relation.c index f92e16e7af8..1158e09c24f 100644 --- i/src/backend/utils/activity/pgstat_relation.c +++ w/src/backend/utils/activity/pgstat_relation.c @@ -158,7 +158,7 @@ pgstat_unlink_relation(Relation rel) return; /* link sanity check */ - Assert(rel->pgstat_info->relation == rel); + Assert(rel->pgstat_info->relation->rd_rel->oid == rel->rd_rel->oid); rel->pgstat_info->relation = NULL; rel->pgstat_info = NULL; } diff --git i/src/backend/utils/cache/relcache.c w/src/backend/utils/cache/relcache.c index bd6cd4e47b5..69237025c20 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); @@ -2666,6 +2667,8 @@ RelationClearRelation(Relation relation, bool rebuild) keep_policies = equalRSDesc(relation->rd_rsdesc, newrel->rd_rsdesc); /* partkey is immutable once set up, so we can always keep it */ keep_partkey = (relation->rd_partkey != NULL); + /* keep stats, except when converting tables into views etc */ + keep_pgstats = relation->rd_rel->relkind == newrel->rd_rel->relkind; /* * Perform swapping of the relcache entry contents. Within this @@ -2720,9 +2723,16 @@ 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); + } + else + pgstat_unlink_relation(relation); + /* preserve old partition key if we have one */ if (keep_partkey) {