tableam scan-API patch broke foreign key validation

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@mail.gmail.com
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.