Re: Failed Assert in pgstat_assoc_relation
Hi, On 2022-12-02 09:51:39 -0800, Andres Freund wrote: > On 2022-12-02 12:15:37 -0500, Tom Lane wrote: > > I wrote: > > > I guess that means that the table-to-view removal has to go in > > > first. I should be able to take care of that tomorrow, or if > > > you're in a hurry I don't mind if you commit it for me. > > > > Done now, feel free to deal with the pgstat problem. > > Thanks. I'm out for a few hours without proper computer access, couldn't > quite get it finished inbetween your push and now. Will deal with it once I > get back. Pushed that now. I debated for a bit whether to backpatch the test all the way, but after it took me a while to convince myself that there's no active problem in the older branches, I decided it's a good idea. Thanks Vignesh for the bugreports! Greetings, Andres Freund
Re: Failed Assert in pgstat_assoc_relation
Hi, On 2022-12-02 12:15:37 -0500, Tom Lane wrote: > I wrote: > > I guess that means that the table-to-view removal has to go in > > first. I should be able to take care of that tomorrow, or if > > you're in a hurry I don't mind if you commit it for me. > > Done now, feel free to deal with the pgstat problem. Thanks. I'm out for a few hours without proper computer access, couldn't quite get it finished inbetween your push and now. Will deal with it once I get back. Greetings, Andres Freund
Re: Failed Assert in pgstat_assoc_relation
I wrote: > I guess that means that the table-to-view removal has to go in > first. I should be able to take care of that tomorrow, or if > you're in a hurry I don't mind if you commit it for me. Done now, feel free to deal with the pgstat problem. regards, tom lane
Re: Failed Assert in pgstat_assoc_relation
Hi, On 2022-12-02 01:03:35 -0500, Tom Lane wrote: > Andres Freund writes: > > On December 1, 2022 9:48:48 PM PST, Tom Lane wrote: > >> I'd suggest putting in an assertion that the relkind isn't changing, > >> instead. > > > Sounds like a plan. Will you do that when you remove the table-to-view > > hack? > > I'd suggest committing it concurrently with the v15 fix, instead, > so that there's a cross-reference to what some future hacker might > need to install if they remove the assertion. Good idea. > I guess that means that the table-to-view removal has to go in > first. I should be able to take care of that tomorrow, or if > you're in a hurry I don't mind if you commit it for me. No particular hurry from my end. Greetings, Andres Freund
Re: Failed Assert in pgstat_assoc_relation
Andres Freund writes: > On December 1, 2022 9:48:48 PM PST, Tom Lane wrote: >> I'd suggest putting in an assertion that the relkind isn't changing, >> instead. > Sounds like a plan. Will you do that when you remove the table-to-view hack? I'd suggest committing it concurrently with the v15 fix, instead, so that there's a cross-reference to what some future hacker might need to install if they remove the assertion. I guess that means that the table-to-view removal has to go in first. I should be able to take care of that tomorrow, or if you're in a hurry I don't mind if you commit it for me. regards, tom lane
Re: Failed Assert in pgstat_assoc_relation
Hi, On December 1, 2022 9:48:48 PM PST, Tom Lane wrote: >Andres Freund writes: >> On 2022-12-02 00:08:20 -0500, Tom Lane wrote: >>> Just the table-to-view hack. I'm not aware that there are any other >>> cases, and it seems hard to credit that there ever will be any. > >> I can see some halfway credible scenarios. E.g. converting a view to a >> matview, or a table into a partition. I kind of wonder if it's worth keeping >> the change, just in case we do - it's not that easy to hit... > >I'd suggest putting in an assertion that the relkind isn't changing, >instead. Sounds like a plan. Will you do that when you remove the table-to-view hack? Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Failed Assert in pgstat_assoc_relation
Andres Freund writes: > On 2022-12-02 00:08:20 -0500, Tom Lane wrote: >> Just the table-to-view hack. I'm not aware that there are any other >> cases, and it seems hard to credit that there ever will be any. > I can see some halfway credible scenarios. E.g. converting a view to a > matview, or a table into a partition. I kind of wonder if it's worth keeping > the change, just in case we do - it's not that easy to hit... I'd suggest putting in an assertion that the relkind isn't changing, instead. When and if somebody makes a credible feature patch that'd require relaxing that, we can see what to do. (There's a couple of places in rewriteHandler.c that could perhaps be simplified, too.) regards, tom lane
Re: Failed Assert in pgstat_assoc_relation
Hi, On 2022-12-02 00:08:20 -0500, Tom Lane wrote: > Andres Freund writes: > > Do we have any cases of relcache entries changing their relkind? > > Just the table-to-view hack. I'm not aware that there are any other > cases, and it seems hard to credit that there ever will be any. I can see some halfway credible scenarios. E.g. converting a view to a matview, or a table into a partition. I kind of wonder if it's worth keeping the change, just in case we do - it's not that easy to hit... > I think we could get rid of table-to-view in HEAD, and use your patch > only in v15. WFM. I'll push it to 15 tomorrow. Greetings, Andres Freund
Re: Failed Assert in pgstat_assoc_relation
Andres Freund writes: > Do we have any cases of relcache entries changing their relkind? Just the table-to-view hack. I'm not aware that there are any other cases, and it seems hard to credit that there ever will be any. I think we could get rid of table-to-view in HEAD, and use your patch only in v15. regards, tom lane
Re: Failed Assert in pgstat_assoc_relation
Hi,
On 2022-11-28 16:33:20 -0500, Tom Lane wrote:
> Andres Freund 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
Re: Failed Assert in pgstat_assoc_relation
Andres Freund 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, especially when you aren't adding that to its API spec (contrast the comments for MemoryContextSetParent). Can't that part be done outside the critical section? regards, tom lane
Re: Failed Assert in pgstat_assoc_relation
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)
{
Re: Failed Assert in pgstat_assoc_relation
Andres Freund writes:
> On 2022-11-28 13:37:16 -0500, Tom Lane wrote:
>> As far as HEAD is concerned, maybe it's time to nuke the whole
>> convert-table-to-view kluge entirely? Only pg_dump older than
>> 9.4 will emit such code, so we're really about out of reasons
>> to keep on maintaining it.
> Sounds good to me.
Here's a draft patch for that. If we apply this to HEAD then
we only need that klugery in relcache for v15.
regards, tom lane
diff --git a/doc/src/sgml/rules.sgml b/doc/src/sgml/rules.sgml
index be3d17c852..3c9459b648 100644
--- a/doc/src/sgml/rules.sgml
+++ b/doc/src/sgml/rules.sgml
@@ -280,14 +280,16 @@
Views in PostgreSQL are implemented
-using the rule system. In fact, there is essentially no difference
-between:
+using the rule system. A view is basically an empty table (having no
+actual storage) with an ON SELECT DO INSTEAD rule.
+Conventionally, that rule is named _RETURN.
+So a view like
CREATE VIEW myview AS SELECT * FROM mytab;
-compared against the two commands:
+is very nearly the same thing as
CREATE TABLE myview (same column list as mytab);
@@ -295,13 +297,17 @@ CREATE RULE "_RETURN" AS ON SELECT TO myview DO INSTEAD
SELECT * FROM mytab;
-because this is exactly what the CREATE VIEW
-command does internally. This has some side effects. One of them
-is that the information about a view in the
-PostgreSQL system catalogs is exactly
-the same as it is for a table. So for the parser, there is
-absolutely no difference between a table and a view. They are the
-same thing: relations.
+although you can't actually write that, because tables are not
+allowed to have ON SELECT rules.
+
+
+
+A view can also have other kinds of DO INSTEAD
+rules, allowing INSERT, UPDATE,
+or DELETE commands to be performed on the view
+despite its lack of underlying storage.
+This is discussed further below, in
+.
@@ -,7 +1117,7 @@ SELECT word FROM words ORDER BY word - 'caterpiler' LIMIT 10;
Rules that are defined on INSERT, UPDATE,
and DELETE are significantly different from the view rules
-described in the previous section. First, their CREATE
+described in the previous sections. First, their CREATE
RULE command allows more:
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index db45d8a08b..8ac2c81b06 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -239,7 +239,6 @@ DefineQueryRewrite(const char *rulename,
Relation event_relation;
ListCell *l;
Query *query;
- bool RelisBecomingView = false;
Oid ruleId = InvalidOid;
ObjectAddress address;
@@ -311,7 +310,18 @@ DefineQueryRewrite(const char *rulename,
/*
* Rules ON SELECT are restricted to view definitions
*
- * So there cannot be INSTEAD NOTHING, ...
+ * So this had better be a view, ...
+ */
+ if (event_relation->rd_rel->relkind != RELKIND_VIEW &&
+ event_relation->rd_rel->relkind != RELKIND_MATVIEW)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("relation \"%s\" cannot have ON SELECT rules",
+ RelationGetRelationName(event_relation)),
+ errdetail_relkind_not_supported(event_relation->rd_rel->relkind)));
+
+ /*
+ * ... there cannot be INSTEAD NOTHING, ...
*/
if (action == NIL)
ereport(ERROR,
@@ -407,93 +417,6 @@ DefineQueryRewrite(const char *rulename,
ViewSelectRuleName)));
rulename = pstrdup(ViewSelectRuleName);
}
-
- /*
- * Are we converting a relation to a view?
- *
- * If so, check that the relation is empty because the storage for the
- * relation is going to be deleted. Also insist that the rel not be
- * involved in partitioning, nor have any triggers, indexes, child or
- * parent tables, RLS policies, or RLS enabled. (Note: some of these
- * tests are too strict, because they will reject relations that once
- * had such but don't anymore. But we don't really care, because this
- * whole business of converting relations to views is just an obsolete
- * kluge to allow dump/reload of views that participate in circular
- * dependencies.)
- */
- if (event_relation->rd_rel->relkind != RELKIND_VIEW &&
- event_relation->rd_rel->relkind != RELKIND_MATVIEW)
- {
- TableScanDesc scanDesc;
- Snapshot snapshot;
- TupleTableSlot *slot;
-
- if (event_relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot convert partitioned table \"%s\" to a view",
-RelationGetRelationName(event_relation;
-
- /* only case left: */
- Assert(event_relation->rd_rel->relkind == RELKIND_RELATION);
-
- if (event_relation->rd_rel->relispartition)
-ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot convert partition \"%s\"
Re: Failed Assert in pgstat_assoc_relation
Hi, On 2022-11-28 13:37:16 -0500, Tom Lane wrote: > vignesh C writes: > > I could reproduce this issue with the following steps: > > create table t1(c int); > > BEGIN; > > CREATE TABLE t (c int); > > SAVEPOINT q; > > CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1; > > select * from t; > > ROLLBACK TO q; > > CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1; > > ROLLBACK; > > 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? > As far as HEAD is concerned, maybe it's time to nuke the whole > convert-table-to-view kluge entirely? Only pg_dump older than > 9.4 will emit such code, so we're really about out of reasons > to keep on maintaining it. Sounds good to me. > However, I'm not sure that removing that code in v15 will fly, Agreed, at the very least that'd increase memory usage. > so maybe we need to make the new pg_stats code a little more > robust against the possibility of a relkind change. Possibly via the relcache code. Greetings, Andres Freund
Re: Failed Assert in pgstat_assoc_relation
vignesh C writes: > I could reproduce this issue with the following steps: > create table t1(c int); > BEGIN; > CREATE TABLE t (c int); > SAVEPOINT q; > CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1; > select * from t; > ROLLBACK TO q; > CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1; > ROLLBACK; 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".) As far as HEAD is concerned, maybe it's time to nuke the whole convert-table-to-view kluge entirely? Only pg_dump older than 9.4 will emit such code, so we're really about out of reasons to keep on maintaining it. However, I'm not sure that removing that code in v15 will fly, so maybe we need to make the new pg_stats code a little more robust against the possibility of a relkind change. regards, tom lane
Re: Failed Assert in pgstat_assoc_relation
On Mon, Nov 28, 2022 at 8:10 PM vignesh C wrote:
>
> Hi,
>
> While reviewing/testing one of the patches I found the following Assert:
> #0 __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=139624429171648) at ./nptl/pthread_kill.c:44
> #1 __pthread_kill_internal (signo=6, threadid=139624429171648) at
> ./nptl/pthread_kill.c:78
> #2 __GI___pthread_kill (threadid=139624429171648,
> signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> #3 0x7efcda6e3476 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4 0x7efcda6c97f3 in __GI_abort () at ./stdlib/abort.c:79
> #5 0x5590bf283139 in ExceptionalCondition
> (conditionName=0x5590bf468170 "rel->pgstat_info->relation == NULL",
> fileName=0x5590bf46812b "pgstat_relation.c", lineNumber=143) at
> assert.c:66
> #6 0x5590bf0ce5f8 in pgstat_assoc_relation (rel=0x7efcce996a48)
> at pgstat_relation.c:143
> #7 0x5590beb83046 in initscan (scan=0x5590bfbf4af8, key=0x0,
> keep_startblock=false) at heapam.c:343
> #8 0x5590beb8466f in heap_beginscan (relation=0x7efcce996a48,
> snapshot=0x5590bfb5a520, nkeys=0, key=0x0, parallel_scan=0x0,
> flags=449) at heapam.c:1223
> #9 0x5590bf02af39 in table_beginscan (rel=0x7efcce996a48,
> snapshot=0x5590bfb5a520, nkeys=0, key=0x0) at
> ../../../src/include/access/tableam.h:891
> #10 0x5590bf02bf8a in DefineQueryRewrite (rulename=0x5590bfb281d0
> "_RETURN", event_relid=16387, event_qual=0x0, event_type=CMD_SELECT,
> is_instead=true, replace=false, action=0x5590bfbf4aa8)
> at rewriteDefine.c:447
> #11 0x5590bf02b5ab in DefineRule (stmt=0x5590bfb285c0,
> queryString=0x5590bfb277a8 "CREATE RULE \"_RETURN\" AS ON SELECT TO t
> DO INSTEAD SELECT * FROM t1;") at rewriteDefine.c:213
>
> I could reproduce this issue with the following steps:
> create table t1(c int);
> BEGIN;
> CREATE TABLE t (c int);
> SAVEPOINT q;
> CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1;
> select * from t;
> ROLLBACK TO q;
> CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1;
> ROLLBACK;
>
> Regards,
> Vignesh
I think what is happening here is that the previous relation is not
unlinked when pgstat_init_relation() is called
because the relation is now a view and for relations without storage
the relation is not unlinked in pgstat_init_relation()
void
pgstat_init_relation(Relation rel)
{
charrelkind = rel->rd_rel->relkind;
/*
* We only count stats for relations with storage and partitioned tables
*/
if (!RELKIND_HAS_STORAGE(relkind) && relkind != RELKIND_PARTITIONED_TABLE)
{
rel->pgstat_enabled = false;
rel->pgstat_info = NULL;
return;
}
There is a logic in DefineQueryRewrite() which converts a relation to
a view when you create such a rule like the test case does.
So initially the relation had storage, the pgstat_info is linked,
then table is converted to a view, but in init, the previous
relation is not unlinked but when it tries to link a new relation, the
assert fails saying a previous relation is already linked to
pgstat_info
I have made a small patch with a fix, but I am not sure if this is the
right way to fix this.
regards,
Ajin Cherian
Fujitsu Australia
pgstat_assoc_fix_for_views.patch
Description: Binary data
Failed Assert in pgstat_assoc_relation
Hi, While reviewing/testing one of the patches I found the following Assert: #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=139624429171648) at ./nptl/pthread_kill.c:44 #1 __pthread_kill_internal (signo=6, threadid=139624429171648) at ./nptl/pthread_kill.c:78 #2 __GI___pthread_kill (threadid=139624429171648, signo=signo@entry=6) at ./nptl/pthread_kill.c:89 #3 0x7efcda6e3476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #4 0x7efcda6c97f3 in __GI_abort () at ./stdlib/abort.c:79 #5 0x5590bf283139 in ExceptionalCondition (conditionName=0x5590bf468170 "rel->pgstat_info->relation == NULL", fileName=0x5590bf46812b "pgstat_relation.c", lineNumber=143) at assert.c:66 #6 0x5590bf0ce5f8 in pgstat_assoc_relation (rel=0x7efcce996a48) at pgstat_relation.c:143 #7 0x5590beb83046 in initscan (scan=0x5590bfbf4af8, key=0x0, keep_startblock=false) at heapam.c:343 #8 0x5590beb8466f in heap_beginscan (relation=0x7efcce996a48, snapshot=0x5590bfb5a520, nkeys=0, key=0x0, parallel_scan=0x0, flags=449) at heapam.c:1223 #9 0x5590bf02af39 in table_beginscan (rel=0x7efcce996a48, snapshot=0x5590bfb5a520, nkeys=0, key=0x0) at ../../../src/include/access/tableam.h:891 #10 0x5590bf02bf8a in DefineQueryRewrite (rulename=0x5590bfb281d0 "_RETURN", event_relid=16387, event_qual=0x0, event_type=CMD_SELECT, is_instead=true, replace=false, action=0x5590bfbf4aa8) at rewriteDefine.c:447 #11 0x5590bf02b5ab in DefineRule (stmt=0x5590bfb285c0, queryString=0x5590bfb277a8 "CREATE RULE \"_RETURN\" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1;") at rewriteDefine.c:213 I could reproduce this issue with the following steps: create table t1(c int); BEGIN; CREATE TABLE t (c int); SAVEPOINT q; CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1; select * from t; ROLLBACK TO q; CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD SELECT * FROM t1; ROLLBACK; Regards, Vignesh
