tableam scan-API patch broke foreign key validation

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

tableam scan-API patch broke foreign key validation

Tom Lane-2
It seems that the fire-the-triggers code path in
validateForeignKeyConstraint isn't being exercised; at least, that's
what coverage.postgresql.org says right now, and I'm afraid that may
have been true for quite some time.  The attached regression-test
addition causes it to be exercised, and guess what: it blows up real
good.

This is a slightly adapted version of the test Hadi proposed in
https://postgr.es/m/CAK=1=WonwcuN_0KiZwQO3SQxse41jZ5hOJRpFCvZ3qa8n9cssw@...
Since he didn't mention anything about core dumps or assertion
failures, one assumes that it did work as of the version he was
testing against.

What it looks like to me is that because of this hunk in c2fe139c2:

@@ -8962,7 +8981,8 @@ validateForeignKeyConstraint(char *conname,
  trigdata.type = T_TriggerData;
  trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
  trigdata.tg_relation = rel;
- trigdata.tg_trigtuple = tuple;
+ trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, true, NULL);
+ trigdata.tg_trigslot = slot;
  trigdata.tg_newtuple = NULL;
  trigdata.tg_trigger = &trig;
 
validateForeignKeyConstraint asks ExecFetchSlotHeapTuple to materialize
the tuple, which causes it to no longer be associated with a buffer,
which causes heapam_tuple_satisfies_snapshot to be very unhappy.

I can make the case not crash by s/true/false/ in the above call,
but I wonder whether that's an appropriate fix.  It seems rather
fragile that things work like this.

I plan to go ahead and commit Hadi's fix with that change included
(as below), but I wonder whether anything else needs to be revisited.

                        regards, tom lane


diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e842f91..31fe40a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4459,13 +4459,8 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
  {
  AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
 
- /*
- * Foreign tables have no storage, nor do partitioned tables and
- * indexes.
- */
- if (tab->relkind == RELKIND_FOREIGN_TABLE ||
- tab->relkind == RELKIND_PARTITIONED_TABLE ||
- tab->relkind == RELKIND_PARTITIONED_INDEX)
+ /* Relations without storage may be ignored here */
+ if (!RELKIND_HAS_STORAGE(tab->relkind))
  continue;
 
  /*
@@ -4645,6 +4640,10 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
  Relation rel = NULL;
  ListCell   *lcon;
 
+ /* Relations without storage may be ignored here too */
+ if (!RELKIND_HAS_STORAGE(tab->relkind))
+ continue;
+
  foreach(lcon, tab->constraints)
  {
  NewConstraint *con = lfirst(lcon);
@@ -9647,7 +9646,7 @@ validateForeignKeyConstraint(char *conname,
  trigdata.type = T_TriggerData;
  trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
  trigdata.tg_relation = rel;
- trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, true, NULL);
+ trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, false, NULL);
  trigdata.tg_trigslot = slot;
  trigdata.tg_newtuple = NULL;
  trigdata.tg_trigger = &trig;
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 620eb43..e62b220 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1895,6 +1895,30 @@ INSERT INTO fk_notpartitioned_pk VALUES (1600, 601), (1600, 1601);
 ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2
   FOR VALUES IN (1600);
 -- leave these tables around intentionally
+-- test the case when the referenced table is owned by a different user
+create role regress_other_partitioned_fk_owner;
+grant references on fk_notpartitioned_pk to regress_other_partitioned_fk_owner;
+set role regress_other_partitioned_fk_owner;
+create table other_partitioned_fk(a int, b int) partition by list (a);
+create table other_partitioned_fk_1 partition of other_partitioned_fk
+  for values in (2048);
+insert into other_partitioned_fk values (2048, 4096);
+-- this should fail
+alter table other_partitioned_fk add foreign key (a, b)
+  references fk_notpartitioned_pk(a, b);
+ERROR:  insert or update on table "other_partitioned_fk_1" violates foreign key constraint "other_partitioned_fk_a_b_fkey"
+DETAIL:  Key (a, b)=(2048, 4096) is not present in table "fk_notpartitioned_pk".
+-- add the missing key and retry
+reset role;
+insert into fk_notpartitioned_pk (a, b) values (2048, 4096);
+set role regress_other_partitioned_fk_owner;
+alter table other_partitioned_fk add foreign key (a, b)
+  references fk_notpartitioned_pk(a, b);
+-- clean up
+drop table other_partitioned_fk;
+reset role;
+revoke all on fk_notpartitioned_pk from regress_other_partitioned_fk_owner;
+drop role regress_other_partitioned_fk_owner;
 -- Test creating a constraint at the parent that already exists in partitions.
 -- There should be no duplicated constraints, and attempts to drop the
 -- constraint in partitions should raise appropriate errors.
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 1a86850..090bd09 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1365,6 +1365,29 @@ ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2
 
 -- leave these tables around intentionally
 
+-- test the case when the referenced table is owned by a different user
+create role regress_other_partitioned_fk_owner;
+grant references on fk_notpartitioned_pk to regress_other_partitioned_fk_owner;
+set role regress_other_partitioned_fk_owner;
+create table other_partitioned_fk(a int, b int) partition by list (a);
+create table other_partitioned_fk_1 partition of other_partitioned_fk
+  for values in (2048);
+insert into other_partitioned_fk values (2048, 4096);
+-- this should fail
+alter table other_partitioned_fk add foreign key (a, b)
+  references fk_notpartitioned_pk(a, b);
+-- add the missing key and retry
+reset role;
+insert into fk_notpartitioned_pk (a, b) values (2048, 4096);
+set role regress_other_partitioned_fk_owner;
+alter table other_partitioned_fk add foreign key (a, b)
+  references fk_notpartitioned_pk(a, b);
+-- clean up
+drop table other_partitioned_fk;
+reset role;
+revoke all on fk_notpartitioned_pk from regress_other_partitioned_fk_owner;
+drop role regress_other_partitioned_fk_owner;
+
 -- Test creating a constraint at the parent that already exists in partitions.
 -- There should be no duplicated constraints, and attempts to drop the
 -- constraint in partitions should raise appropriate errors.
Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Andres Freund
Hi,

On April 6, 2019 11:07:55 AM PDT, Tom Lane <[hidden email]> wrote:

>It seems that the fire-the-triggers code path in
>validateForeignKeyConstraint isn't being exercised; at least, that's
>what coverage.postgresql.org says right now, and I'm afraid that may
>have been true for quite some time.  The attached regression-test
>addition causes it to be exercised, and guess what: it blows up real
>good.
>
>This is a slightly adapted version of the test Hadi proposed in
>https://postgr.es/m/CAK=1=WonwcuN_0KiZwQO3SQxse41jZ5hOJRpFCvZ3qa8n9cssw@...
>Since he didn't mention anything about core dumps or assertion
>failures, one assumes that it did work as of the version he was
>testing against.
>
>What it looks like to me is that because of this hunk in c2fe139c2:
>
>@@ -8962,7 +8981,8 @@ validateForeignKeyConstraint(char *conname,
> trigdata.type = T_TriggerData;
> trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
> trigdata.tg_relation = rel;
>- trigdata.tg_trigtuple = tuple;
>+ trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, true, NULL);
>+ trigdata.tg_trigslot = slot;
> trigdata.tg_newtuple = NULL;
> trigdata.tg_trigger = &trig;
>
>validateForeignKeyConstraint asks ExecFetchSlotHeapTuple to materialize
>the tuple, which causes it to no longer be associated with a buffer,
>which causes heapam_tuple_satisfies_snapshot to be very unhappy.
>
>I can make the case not crash by s/true/false/ in the above call,
>but I wonder whether that's an appropriate fix.  It seems rather
>fragile that things work like this.
>
>I plan to go ahead and commit Hadi's fix with that change included
>(as below), but I wonder whether anything else needs to be revisited.

I posted pretty much that patch nearby, with some other questions. Was waiting for David to respond.... Let me dig that out.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.


Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On April 6, 2019 11:07:55 AM PDT, Tom Lane <[hidden email]> wrote:
>> I plan to go ahead and commit Hadi's fix with that change included
>> (as below), but I wonder whether anything else needs to be revisited.

> I posted pretty much that patch nearby, with some other questions. Was waiting for David to respond.... Let me dig that out.

Ah.  Would you rather I wait till you push yours?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Andres Freund
Hi,

On 2019-04-06 14:13:29 -0400, Tom Lane wrote:
> Andres Freund <[hidden email]> writes:
> > On April 6, 2019 11:07:55 AM PDT, Tom Lane <[hidden email]> wrote:
> >> I plan to go ahead and commit Hadi's fix with that change included
> >> (as below), but I wonder whether anything else needs to be revisited.
>
> > I posted pretty much that patch nearby, with some other questions. Was waiting for David to respond.... Let me dig that out.

The relevant thread is:
https://www.postgresql.org/message-id/20190325180405.jytoehuzkeozggxx%40alap3.anarazel.de


> Ah.  Would you rather I wait till you push yours?

Yours looks good to me, so go ahead. I think we need a bit more than
that, but that can easily be committed separately:

Wonder if you have an opinion on:

> I've also noticed that we should free the tuple - that doesn't matter
> for heap, but it sure does for other callers.  But uh, is it actually ok
> to validate an entire table's worth of foreign keys without a memory
> context reset? I.e. shouldn't we have a memory context that we reset
> after each iteration?
>
> Also, why's there no CHECK_FOR_INTERRUPTS()? heap has some internally on
> a page level, but that doesn't seem all that granular?


Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Tom Lane-2
Andres Freund <[hidden email]> writes:
> The relevant thread is:
> https://www.postgresql.org/message-id/20190325180405.jytoehuzkeozggxx%40alap3.anarazel.de

Yeah, I just found that --- would have seen it sooner if David had
not elected to make it a new thread.

> Wonder if you have an opinion on:

>> I've also noticed that we should free the tuple - that doesn't matter
>> for heap, but it sure does for other callers.

Why should this code need to free anything?  That'd be the responsibility
of the slot code, no?

>> But uh, is it actually ok
>> to validate an entire table's worth of foreign keys without a memory
>> context reset? I.e. shouldn't we have a memory context that we reset
>> after each iteration?
>> Also, why's there no CHECK_FOR_INTERRUPTS()? heap has some internally on
>> a page level, but that doesn't seem all that granular?

These are good questions.  Just eyeing RI_FKey_check(), I think
that it might not have any significant leaks because most of the work
is done in an SPI context, but obviously that's pretty fragile.

The memory-context stuff in your WIP patch seems wrong, btw;
the second or later iteration of the loop would trash oldcxt.

But clearly we need a test case here.  I'll adjust Hadi's example
so that there's more than one tuple to check, and push it.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Andres Freund
Hi,

On 2019-04-06 14:34:34 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > The relevant thread is:
> > https://www.postgresql.org/message-id/20190325180405.jytoehuzkeozggxx%40alap3.anarazel.de
>
> Yeah, I just found that --- would have seen it sooner if David had
> not elected to make it a new thread.
>
> > Wonder if you have an opinion on:
>
> >> I've also noticed that we should free the tuple - that doesn't matter
> >> for heap, but it sure does for other callers.
>
> Why should this code need to free anything?  That'd be the responsibility
> of the slot code, no?

Well, not really. If a slot doesn't hold heap tuples internally,
ExecFetchSlotHeapTuple() will return a fresh heap tuple (but signal so
by setting *should_free = true if not NULL).  That's why I was saying it
doesn't matter for heap (where the slot just holds a heap tuple
internally), but it does matter for other AMs.


> >> But uh, is it actually ok
> >> to validate an entire table's worth of foreign keys without a memory
> >> context reset? I.e. shouldn't we have a memory context that we reset
> >> after each iteration?
> >> Also, why's there no CHECK_FOR_INTERRUPTS()? heap has some internally on
> >> a page level, but that doesn't seem all that granular?
>
> These are good questions.  Just eyeing RI_FKey_check(), I think
> that it might not have any significant leaks because most of the work
> is done in an SPI context, but obviously that's pretty fragile.

Yea. And especially with potentially needing to free the tuple as above,
using an explicit context seems more robust to me.


> But clearly we need a test case here.  I'll adjust Hadi's example
> so that there's more than one tuple to check, and push it.

Cool.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2019-04-06 14:34:34 -0400, Tom Lane wrote:
>> Why should this code need to free anything?  That'd be the responsibility
>> of the slot code, no?

> Well, not really. If a slot doesn't hold heap tuples internally,
> ExecFetchSlotHeapTuple() will return a fresh heap tuple (but signal so
> by setting *should_free = true if not NULL).

Ah, got it: ignoring should_free is indeed a potential issue here.

>> But clearly we need a test case here.  I'll adjust Hadi's example
>> so that there's more than one tuple to check, and push it.

> Cool.

Sounds like a plan.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> writes:
> On 2019-04-06 14:34:34 -0400, Tom Lane wrote:
>> These are good questions.  Just eyeing RI_FKey_check(), I think
>> that it might not have any significant leaks because most of the work
>> is done in an SPI context, but obviously that's pretty fragile.

> Yea. And especially with potentially needing to free the tuple as above,
> using an explicit context seems more robust to me.

Adjusting the committed test case to process lots of tuples shows that
indeed there is no leak in HEAD.  But I concur that using a local context
here would be more future-proof.

BTW, I just stumbled across a different bug in v11 by trying to run
HEAD's test script on it ... not sure if that's a known problem or not:

(gdb) f 3
#3  0x000000000063949c in ExecSetupPartitionTupleRouting (
    mtstate=<value optimized out>, rel=0x7f343e4f4170) at execPartition.c:201
201             Assert(update_rri_index == num_update_rri);
(gdb) bt
#0  0x00000037b6232495 in raise (sig=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00000037b6233c75 in abort () at abort.c:92
#2  0x00000000008a1e6d in ExceptionalCondition (
    conditionName=<value optimized out>, errorType=<value optimized out>,
    fileName=<value optimized out>, lineNumber=<value optimized out>)
    at assert.c:54
#3  0x000000000063949c in ExecSetupPartitionTupleRouting (
    mtstate=<value optimized out>, rel=0x7f343e4f4170) at execPartition.c:201
#4  0x00000000006595cb in ExecInitModifyTable (node=0x26a0680,
    estate=0x26a1fa8, eflags=16) at nodeModifyTable.c:2343
#5  0x000000000063b179 in ExecInitNode (node=0x26a0680, estate=0x26a1fa8,
    eflags=16) at execProcnode.c:174
#6  0x0000000000635824 in InitPlan (queryDesc=<value optimized out>, eflags=16)
    at execMain.c:1046
#7  standard_ExecutorStart (queryDesc=<value optimized out>, eflags=16)
    at execMain.c:265
#8  0x000000000066c332 in _SPI_pquery (plan=0x269fb38, paramLI=0x26b9048,
    snapshot=0x0, crosscheck_snapshot=0x0, read_only=false,
    fire_triggers=false, tcount=0) at spi.c:2482
#9  _SPI_execute_plan (plan=0x269fb38, paramLI=0x26b9048, snapshot=0x0,
    crosscheck_snapshot=0x0, read_only=false, fire_triggers=false, tcount=0)
    at spi.c:2246
#10 0x000000000066c7b6 in SPI_execute_snapshot (plan=0x269fb38,
    Values=<value optimized out>, Nulls=<value optimized out>, snapshot=0x0,
    crosscheck_snapshot=0x0, read_only=false, fire_triggers=false, tcount=0)
    at spi.c:562
#11 0x0000000000838842 in ri_PerformCheck (riinfo=0x268d9c0,
    qkey=0x7fff8996f700, qplan=0x269fb38, fk_rel=0x7f343e4f4170,
    pk_rel=0x7f343e4f49d0, old_tuple=0x7fff8996fd40, new_tuple=0x0,
    detectNewRows=true, expect_OK=9) at ri_triggers.c:2606
#12 0x0000000000839971 in ri_setnull (trigdata=<value optimized out>)
    at ri_triggers.c:1400
#13 0x000000000060b0a8 in ExecCallTriggerFunc (trigdata=0x7fff8996fce0,
    tgindx=0, finfo=0x26c55e0, instr=0x0, per_tuple_context=0x26aeef0)
    at trigger.c:2412
#14 0x000000000060b5e5 in AfterTriggerExecute (events=0x260b8d8, firing_id=1,
    estate=0x26c5098, delete_ok=false) at trigger.c:4359
#15 afterTriggerInvokeEvents (events=0x260b8d8, firing_id=1, estate=0x26c5098,
    delete_ok=false) at trigger.c:4550
#16 0x000000000060cb82 in AfterTriggerEndQuery (estate=0x26c5098)
    at trigger.c:4860
#17 0x0000000000636871 in standard_ExecutorFinish (queryDesc=0x2717b88)
    at execMain.c:439
#18 0x0000000000795bf8 in ProcessQuery (plan=<value optimized out>,
    sourceText=0x258ef98 "DELETE FROM fk_notpartitioned_pk;", params=0x0,
    queryEnv=0x0, dest=<value optimized out>,
    completionTag=0x7fff89970030 "DELETE 2") at pquery.c:205
#19 0x0000000000795e35 in PortalRunMulti (portal=0x25f4518, isTopLevel=true,
    setHoldSnapshot=false, dest=0x271bb90, altdest=0x271bb90,
    completionTag=0x7fff89970030 "DELETE 2") at pquery.c:1286
#20 0x0000000000796610 in PortalRun (portal=0x25f4518,
    count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x271bb90,
    altdest=0x271bb90, completionTag=0x7fff89970030 "DELETE 2") at pquery.c:799
#21 0x00000000007929dd in exec_simple_query (
    query_string=0x258ef98 "DELETE FROM fk_notpartitioned_pk;")
    at postgres.c:1145
#22 0x0000000000793f34 in PostgresMain (argc=<value optimized out>,
    argv=<value optimized out>, dbname=0x25b8a18 "regression",
    username=<value optimized out>) at postgres.c:4182


                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2019-04-06 14:43:26 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2019-04-06 14:34:34 -0400, Tom Lane wrote:
> >> Why should this code need to free anything?  That'd be the responsibility
> >> of the slot code, no?
>
> > Well, not really. If a slot doesn't hold heap tuples internally,
> > ExecFetchSlotHeapTuple() will return a fresh heap tuple (but signal so
> > by setting *should_free = true if not NULL).
>
> Ah, got it: ignoring should_free is indeed a potential issue here.

I've pushed a revised version of my earlier patch adding a memory
context that's reset after each tuple.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: tableam scan-API patch broke foreign key validation

Alvaro Herrera-9
In reply to this post by Tom Lane-2
On 2019-Apr-06, Tom Lane wrote:

> BTW, I just stumbled across a different bug in v11 by trying to run
> HEAD's test script on it ... not sure if that's a known problem or not:
>
> (gdb) f 3
> #3  0x000000000063949c in ExecSetupPartitionTupleRouting (
>     mtstate=<value optimized out>, rel=0x7f343e4f4170) at execPartition.c:201
> 201             Assert(update_rri_index == num_update_rri);
> (gdb) bt
> #0  0x00000037b6232495 in raise (sig=6)
>     at ../nptl/sysdeps/unix/sysv/linux/raise.c:64

For closure: this was re-reported as
https://www.postgresql.org/message-id/20710.1554582479@...
and the fix committed as 10e3991fad8a300ed268878ae30c96074628c1e1.

Thanks

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services