parent foreign tables and row marks
I noticed that 428b260f87e8 (v12) broke the cases where a parent
foreign table has row marks assigned. Specifically, the following
Assert added to expand_inherited_rtentry() by that commit looks bogus
in this regard:
/* The old PlanRowMark should already have necessitated adding TID */
Assert(old_allMarkTypes & ~(1 << ROW_MARK_COPY));
The Assert appears to have been written based on the assumption that
the root parent would always be a local heap relation, but given that
we allow foreign tables also to be inheritance parents, that
assumption is false.
Problem cases:
create extension postgres_fdw ;
create server loopback foreign data wrapper postgres_fdw;
create user mapping for current_user server loopback ;
create table loct1 (a int);
create foreign table ft_parent (a int) server loopback options
(table_name 'loct1');
create table loct2 (a int);
create foreign table ft_child () inherits (ft_parent) server loopback
options (table_name 'loct2');
explain (verbose) select * from ft_parent FOR UPDATE;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!?> \q
Just commenting out that Assert will let the above work, but that's
not enough, because any child tables that are local can't access ctidN
junk columns that should have been added before reaching
expand_inherited_rtentry(), but wouldn't because the parent is a
foreign table.
create table loct3 () inherits (ft_parent);
explain (verbose) select * from ft_parent FOR UPDATE;
ERROR: could not find junk ctid1 column
The right thing would have been to have the same code as in
preprocess_targetlist() to add a TID row marking junk column if
needed. Attached a patch for that, which also adds the test cases.
Actually, I had to make a separate version of the patch to apply to
the v12 branch, because EXPLAIN outputs relation aliases a bit
differently starting in v13, which is attached too.
--
Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
inheritance-rowmark-ctid-fix_PG12.patchapplication/octet-stream; name=inheritance-rowmark-ctid-fix_PG12.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 252b8dab60..83f0954ee5 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7190,6 +7190,95 @@ select * from bar where f1 in (select f1 from foo) for share;
4 | 44
(4 rows)
+-- Now check SELECT FOR UPDATE/SHARE with an inherited source table,
+-- where the parent is itself a foreign table
+create table loct4 (f1 int, f2 int, f3 int);
+create foreign table foo2child (f3 int) inherits (foo2)
+ server loopback options (table_name 'loct4');
+NOTICE: moving and merging column "f3" with inherited definition
+DETAIL: User-specified column moved to the position of the inherited column.
+explain (verbose, costs off)
+select * from bar where f1 in (select f1 from foo2) for share;
+ QUERY PLAN
+---------------------------------------------------------------------------------------
+ LockRows
+ Output: bar.f1, bar.f2, bar.ctid, foo2.*, bar.*, bar.tableoid, foo2.tableoid
+ -> Hash Join
+ Output: bar.f1, bar.f2, bar.ctid, foo2.*, bar.*, bar.tableoid, foo2.tableoid
+ Inner Unique: true
+ Hash Cond: (bar.f1 = foo2.f1)
+ -> Append
+ -> Seq Scan on public.bar
+ Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid
+ -> Foreign Scan on public.bar2
+ Output: bar2.f1, bar2.f2, bar2.ctid, bar2.*, bar2.tableoid
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR SHARE
+ -> Hash
+ Output: foo2.*, foo2.f1, foo2.tableoid
+ -> HashAggregate
+ Output: foo2.*, foo2.f1, foo2.tableoid
+ Group Key: foo2.f1
+ -> Append
+ -> Foreign Scan on public.foo2
+ Output: foo2.*, foo2.f1, foo2.tableoid
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct1
+ -> Foreign Scan on public.foo2child
+ Output: foo2child.*, foo2child.f1, foo2child.tableoid
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct4
+(24 rows)
+
+select * from bar where f1 in (select f1 from foo2) for share;
+ f1 | f2
+----+----
+ 2 | 22
+ 4 | 44
+(2 rows)
+
+-- And with a local child relation of the foreign table parent
+create table foo2child2 (f3 int) inherits (foo2);
+NOTICE: moving and merging column "f3" with inherited definition
+DETAIL: User-specified column moved to the position of the inherited column.
+explain (verbose, costs off)
+select * from bar where f1 in (select f1 from foo2) for share;
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ LockRows
+ Output: bar.f1, bar.f2, bar.ctid, foo2.*, bar.*, bar.tableoid, foo2.ctid, foo2.tableoid
+ -> Hash Join
+ Output: bar.f1, bar.f2, bar.ctid, foo2.*, bar.*, bar.tableoid, foo2.ctid, foo2.tableoid
+ Inner Unique: true
+ Hash Cond: (bar.f1 = foo2.f1)
+ -> Append
+ -> Seq Scan on public.bar
+ Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid
+ -> Foreign Scan on public.bar2
+ Output: bar2.f1, bar2.f2, bar2.ctid, bar2.*, bar2.tableoid
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR SHARE
+ -> Hash
+ Output: foo2.*, foo2.f1, foo2.ctid, foo2.tableoid
+ -> HashAggregate
+ Output: foo2.*, foo2.f1, foo2.ctid, foo2.tableoid
+ Group Key: foo2.f1
+ -> Append
+ -> Foreign Scan on public.foo2
+ Output: foo2.*, foo2.f1, foo2.ctid, foo2.tableoid
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1
+ -> Foreign Scan on public.foo2child
+ Output: foo2child.*, foo2child.f1, foo2child.ctid, foo2child.tableoid
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct4
+ -> Seq Scan on public.foo2child2
+ Output: foo2child2.*, foo2child2.f1, foo2child2.ctid, foo2child2.tableoid
+(26 rows)
+
+select * from bar where f1 in (select f1 from foo2) for share;
+ f1 | f2
+----+----
+ 2 | 22
+ 4 | 44
+(2 rows)
+
+alter table foo2child no inherit foo2;
+alter table foo2child2 no inherit foo2;
-- Check UPDATE with inherited target and an inherited source table
explain (verbose, costs off)
update bar set f2 = f2 + 100 where f1 in (select f1 from foo);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index a957ef1b1d..88c343295a 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1834,6 +1834,26 @@ explain (verbose, costs off)
select * from bar where f1 in (select f1 from foo) for share;
select * from bar where f1 in (select f1 from foo) for share;
+-- Now check SELECT FOR UPDATE/SHARE with an inherited source table,
+-- where the parent is itself a foreign table
+create table loct4 (f1 int, f2 int, f3 int);
+create foreign table foo2child (f3 int) inherits (foo2)
+ server loopback options (table_name 'loct4');
+
+explain (verbose, costs off)
+select * from bar where f1 in (select f1 from foo2) for share;
+select * from bar where f1 in (select f1 from foo2) for share;
+
+-- And with a local child relation of the foreign table parent
+create table foo2child2 (f3 int) inherits (foo2);
+
+explain (verbose, costs off)
+select * from bar where f1 in (select f1 from foo2) for share;
+select * from bar where f1 in (select f1 from foo2) for share;
+
+alter table foo2child no inherit foo2;
+alter table foo2child2 no inherit foo2;
+
-- Check UPDATE with inherited target and an inherited source table
explain (verbose, costs off)
update bar set f2 = f2 + 100 where f1 in (select f1 from foo);
diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c
index f0953475b1..ad0e91e2c5 100644
--- a/src/backend/optimizer/util/inherit.c
+++ b/src/backend/optimizer/util/inherit.c
@@ -231,9 +231,25 @@ expand_inherited_rtentry(PlannerInfo *root, RelOptInfo *rel,
char resname[32];
List *newvars = NIL;
- /* The old PlanRowMark should already have necessitated adding TID */
- Assert(old_allMarkTypes & ~(1 << ROW_MARK_COPY));
-
+ /* Add TID if needed, unless we had it already */
+ if (new_allMarkTypes & ~(1 << ROW_MARK_COPY) &&
+ !(old_allMarkTypes & ~(1 << ROW_MARK_COPY)))
+ {
+ /* Need to fetch TID */
+ var = makeVar(oldrc->rti,
+ SelfItemPointerAttributeNumber,
+ TIDOID,
+ -1,
+ InvalidOid,
+ 0);
+ snprintf(resname, sizeof(resname), "ctid%u", oldrc->rowmarkId);
+ tle = makeTargetEntry((Expr *) var,
+ list_length(root->processed_tlist) + 1,
+ pstrdup(resname),
+ true);
+ root->processed_tlist = lappend(root->processed_tlist, tle);
+ newvars = lappend(newvars, var);
+ }
/* Add whole-row junk Var if needed, unless we had it already */
if ((new_allMarkTypes & (1 << ROW_MARK_COPY)) &&
!(old_allMarkTypes & (1 << ROW_MARK_COPY)))
inheritance-rowmark-ctid-fix.patchapplication/octet-stream; name=inheritance-rowmark-ctid-fix.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 7df30010f2..efe1bf0e79 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7255,6 +7255,95 @@ select * from bar where f1 in (select f1 from foo) for share;
4 | 44
(4 rows)
+-- Now check SELECT FOR UPDATE/SHARE with an inherited source table,
+-- where the parent is itself a foreign table
+create table loct4 (f1 int, f2 int, f3 int);
+create foreign table foo2child (f3 int) inherits (foo2)
+ server loopback options (table_name 'loct4');
+NOTICE: moving and merging column "f3" with inherited definition
+DETAIL: User-specified column moved to the position of the inherited column.
+explain (verbose, costs off)
+select * from bar where f1 in (select f1 from foo2) for share;
+ QUERY PLAN
+--------------------------------------------------------------------------------------
+ LockRows
+ Output: bar.f1, bar.f2, bar.ctid, foo2.*, bar.*, bar.tableoid, foo2.tableoid
+ -> Hash Join
+ Output: bar.f1, bar.f2, bar.ctid, foo2.*, bar.*, bar.tableoid, foo2.tableoid
+ Inner Unique: true
+ Hash Cond: (bar.f1 = foo2.f1)
+ -> Append
+ -> Seq Scan on public.bar bar_1
+ Output: bar_1.f1, bar_1.f2, bar_1.ctid, bar_1.*, bar_1.tableoid
+ -> Foreign Scan on public.bar2 bar_2
+ Output: bar_2.f1, bar_2.f2, bar_2.ctid, bar_2.*, bar_2.tableoid
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR SHARE
+ -> Hash
+ Output: foo2.*, foo2.f1, foo2.tableoid
+ -> HashAggregate
+ Output: foo2.*, foo2.f1, foo2.tableoid
+ Group Key: foo2.f1
+ -> Append
+ -> Foreign Scan on public.foo2 foo2_1
+ Output: foo2_1.*, foo2_1.f1, foo2_1.tableoid
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct1
+ -> Foreign Scan on public.foo2child foo2_2
+ Output: foo2_2.*, foo2_2.f1, foo2_2.tableoid
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct4
+(24 rows)
+
+select * from bar where f1 in (select f1 from foo2) for share;
+ f1 | f2
+----+----
+ 2 | 22
+ 4 | 44
+(2 rows)
+
+-- And with a local child relation of the foreign table parent
+create table foo2child2 (f3 int) inherits (foo2);
+NOTICE: moving and merging column "f3" with inherited definition
+DETAIL: User-specified column moved to the position of the inherited column.
+explain (verbose, costs off)
+select * from bar where f1 in (select f1 from foo2) for share;
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------
+ LockRows
+ Output: bar.f1, bar.f2, bar.ctid, foo2.*, bar.*, bar.tableoid, foo2.ctid, foo2.tableoid
+ -> Hash Join
+ Output: bar.f1, bar.f2, bar.ctid, foo2.*, bar.*, bar.tableoid, foo2.ctid, foo2.tableoid
+ Inner Unique: true
+ Hash Cond: (bar.f1 = foo2.f1)
+ -> Append
+ -> Seq Scan on public.bar bar_1
+ Output: bar_1.f1, bar_1.f2, bar_1.ctid, bar_1.*, bar_1.tableoid
+ -> Foreign Scan on public.bar2 bar_2
+ Output: bar_2.f1, bar_2.f2, bar_2.ctid, bar_2.*, bar_2.tableoid
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR SHARE
+ -> Hash
+ Output: foo2.*, foo2.f1, foo2.ctid, foo2.tableoid
+ -> HashAggregate
+ Output: foo2.*, foo2.f1, foo2.ctid, foo2.tableoid
+ Group Key: foo2.f1
+ -> Append
+ -> Foreign Scan on public.foo2 foo2_1
+ Output: foo2_1.*, foo2_1.f1, foo2_1.ctid, foo2_1.tableoid
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1
+ -> Foreign Scan on public.foo2child foo2_2
+ Output: foo2_2.*, foo2_2.f1, foo2_2.ctid, foo2_2.tableoid
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct4
+ -> Seq Scan on public.foo2child2 foo2_3
+ Output: foo2_3.*, foo2_3.f1, foo2_3.ctid, foo2_3.tableoid
+(26 rows)
+
+select * from bar where f1 in (select f1 from foo2) for share;
+ f1 | f2
+----+----
+ 2 | 22
+ 4 | 44
+(2 rows)
+
+alter table foo2child no inherit foo2;
+alter table foo2child2 no inherit foo2;
-- Check UPDATE with inherited target and an inherited source table
explain (verbose, costs off)
update bar set f2 = f2 + 100 where f1 in (select f1 from foo);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 78379bdea5..3aebe91d3c 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1891,6 +1891,26 @@ explain (verbose, costs off)
select * from bar where f1 in (select f1 from foo) for share;
select * from bar where f1 in (select f1 from foo) for share;
+-- Now check SELECT FOR UPDATE/SHARE with an inherited source table,
+-- where the parent is itself a foreign table
+create table loct4 (f1 int, f2 int, f3 int);
+create foreign table foo2child (f3 int) inherits (foo2)
+ server loopback options (table_name 'loct4');
+
+explain (verbose, costs off)
+select * from bar where f1 in (select f1 from foo2) for share;
+select * from bar where f1 in (select f1 from foo2) for share;
+
+-- And with a local child relation of the foreign table parent
+create table foo2child2 (f3 int) inherits (foo2);
+
+explain (verbose, costs off)
+select * from bar where f1 in (select f1 from foo2) for share;
+select * from bar where f1 in (select f1 from foo2) for share;
+
+alter table foo2child no inherit foo2;
+alter table foo2child2 no inherit foo2;
+
-- Check UPDATE with inherited target and an inherited source table
explain (verbose, costs off)
update bar set f2 = f2 + 100 where f1 in (select f1 from foo);
diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c
index 13f67ab744..1a7d475848 100644
--- a/src/backend/optimizer/util/inherit.c
+++ b/src/backend/optimizer/util/inherit.c
@@ -232,9 +232,25 @@ expand_inherited_rtentry(PlannerInfo *root, RelOptInfo *rel,
char resname[32];
List *newvars = NIL;
- /* The old PlanRowMark should already have necessitated adding TID */
- Assert(old_allMarkTypes & ~(1 << ROW_MARK_COPY));
-
+ /* Add TID if needed, unless we had it already */
+ if (new_allMarkTypes & ~(1 << ROW_MARK_COPY) &&
+ !(old_allMarkTypes & ~(1 << ROW_MARK_COPY)))
+ {
+ /* Need to fetch TID */
+ var = makeVar(oldrc->rti,
+ SelfItemPointerAttributeNumber,
+ TIDOID,
+ -1,
+ InvalidOid,
+ 0);
+ snprintf(resname, sizeof(resname), "ctid%u", oldrc->rowmarkId);
+ tle = makeTargetEntry((Expr *) var,
+ list_length(root->processed_tlist) + 1,
+ pstrdup(resname),
+ true);
+ root->processed_tlist = lappend(root->processed_tlist, tle);
+ newvars = lappend(newvars, var);
+ }
/* Add whole-row junk Var if needed, unless we had it already */
if ((new_allMarkTypes & (1 << ROW_MARK_COPY)) &&
!(old_allMarkTypes & (1 << ROW_MARK_COPY)))
Amit Langote <amitlangote09@gmail.com> writes:
I noticed that 428b260f87e8 (v12) broke the cases where a parent
foreign table has row marks assigned.
Indeed :-(. Fix pushed. I tweaked the comments and test case slightly.
regards, tom lane
On Thu, Jun 3, 2021 at 3:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Amit Langote <amitlangote09@gmail.com> writes:
I noticed that 428b260f87e8 (v12) broke the cases where a parent
foreign table has row marks assigned.Indeed :-(. Fix pushed. I tweaked the comments and test case slightly.
Thank you.
--
Amit Langote
EDB: http://www.enterprisedb.com
On Thu, Jun 3, 2021 at 10:07 AM Amit Langote <amitlangote09@gmail.com> wrote:
On Thu, Jun 3, 2021 at 3:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Amit Langote <amitlangote09@gmail.com> writes:
I noticed that 428b260f87e8 (v12) broke the cases where a parent
foreign table has row marks assigned.Indeed :-(. Fix pushed. I tweaked the comments and test case slightly.
Thank you.
Ah, I had forgotten to propose that we replace the following in the
preprocess_targetlist()'s row marks loop:
/* child rels use the same junk attrs as their parents */
if (rc->rti != rc->prti)
continue;
by an Assert as follows:
+ /* No child row marks yet. */
+ Assert (rc->rti == rc->prti);
I think the only place that sets prti that is != rti of a row mark is
expand_single_inheritance_child() and we can be sure that that
function now always runs after preprocess_targetlist() has run.
Attached a patch.
Thoughts?
--
Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
no-child-rowmarks.patchapplication/octet-stream; name=no-child-rowmarks.patchDownload
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index e9434580d6..895bcd5388 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -146,9 +146,8 @@ preprocess_targetlist(PlannerInfo *root)
char resname[32];
TargetEntry *tle;
- /* child rels use the same junk attrs as their parents */
- if (rc->rti != rc->prti)
- continue;
+ /* No child row marks yet. */
+ Assert (rc->rti == rc->prti);
if (rc->allMarkTypes & ~(1 << ROW_MARK_COPY))
{