Prevent pgstats from getting confused when relkind of a relation changes

When the relkind of a relache entry changes, because a table is converted into
a view, pgstats can get confused in 15+, leading to crashes or assertion
failures.

For HEAD, Tom fixed this in b23cd185fd5, by removing support for converting a
table to a view, removing the source of the inconsistency. This commit just
adds an assertion that a relcache entry's relkind does not change, just in
case we end up with another case of that in the future. As there's no cases of
changing relkind anymore, we can't add a test that that's handled correctly.

For 15, fix the problem by not maintaining the association with the old pgstat
entry when the relkind changes during a relcache invalidation processing. In
that case the pgstat entry needs to be unlinked first, to avoid
PgStat_TableStatus->relation getting out of sync. Also add a test reproducing
the issues.

No known problem exists in 11-14, so just add the test there.

Reported-by: vignesh C <vignesh21@gmail.com>
Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CALDaNm2yXz+zOtv7y5zBd5WKT8O0Ld3YxikuU3dcyCvxF7gypA@mail.gmail.com
Discussion: https://postgr.es/m/CALDaNm3oZA-8Wbps2Jd1g5_Gjrr-x3YWrJPek-mF5Asrrvz2Dg@mail.gmail.com
Backpatch: 15-

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index f8d9f5fa485818394f5141a1df3651af1b36bba6..52fe7a744530fc30cb42ff76b425f7fff87f2a5d 100644 (file)
--- a/src/backend/utils/cache/relcache.c
+++ b/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. when
+                * 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 a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index 11814bfd06f2a67bc51b898739ab5b8fbb0f705f..63c3c2aa9af7b7b7a1fba1f5ee5c6c23928b8144 100644 (file)
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -2122,6 +2122,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+zOtv7y5zBd5WKT8O0Ld3YxikuU3dcyCvxF7gypA@mail.gmail.com
+-- CALDaNm3oZA-8Wbps2Jd1g5_Gjrr-x3YWrJPek-mF5Asrrvz2Dg@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
@@ -2153,7 +2176,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
@@ -2231,3 +2254,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 a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index 2e0288c5705c517a27c4cb70259cdacab7744d2a..ee28d4502d4a504c8ce82442b3d85e0c615a40e1 100644 (file)
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -781,6 +781,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+zOtv7y5zBd5WKT8O0Ld3YxikuU3dcyCvxF7gypA@mail.gmail.com
+-- CALDaNm3oZA-8Wbps2Jd1g5_Gjrr-x3YWrJPek-mF5Asrrvz2Dg@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;