preprocess_targetlist and inheiritance

Started by Stephen Frostover 10 years ago4 messages

sfrost@snowman.net

over 10 years ago

1 attachment(s)

Tom, all,

Looks like preprocess_targetlist() should have been adjusted with the
changes to ExecBuildAuxRowMark() to support foreign tables being part
of inheritance trees (cb1ca4d800621dcae67ca6c799006de99fa4f0a5) to
also include the tableoid regardless of the rowMark type, if the
relation is the parent of an inheritance tree.

This was noted by Dean Rasheed while working on RLS since it was
causing one of the new RLS-with-inheritance regression tests to fail
with: ERROR: could not find junk tableoid1 column

This does change the output a bit in the regression tests due to the
change in ordering of the columns displayed by explain.

Patch attached for your review.

Thoughts?

I'm happy to push this if no one has any issues with it, but also
won't object if you'd prefer to.

Thanks!

Stephen

Attachments:

preprocess_targetlist_inheritance.patchtext/x-diff; charset=us-asciiDownload

From c7af0f658666769f010643bba9c7467ddc42c13c Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Tue, 21 Apr 2015 09:52:09 -0400
Subject: [PATCH] Pull in tableoid for inheiritance with rowMarks

As noted by Dean Rasheed[1], cb1ca4d800621dcae67ca6c799006de99fa4f0a5
changed ExecBuildAuxRowMark() to always look for the tableoid in the
target list, but didn't also change preprocess_targetlist() to always
include the tableoid.  This resulted in errors with soon-to-be-added RLS
with inheritance tests, though I suspect there could be other issues
arising from this.

Pushing this independently as it's not directly related to the other RLS
changes which are coming.

Author: Dean Rasheed (extracted from his rls.v6.patch).

[1] CAEZATCVmFUfUOwwhnBTcgi6AquyjQ0-1fyKd0T3xBWJvn+xsFA@mail.gmail.com
---
 contrib/postgres_fdw/expected/postgres_fdw.out | 52 +++++++++++++-------------
 src/backend/optimizer/prep/preptlist.c         | 34 ++++++++---------
 2 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 783cb41..93e9836 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -3193,26 +3193,26 @@ select * from bar where f1 in (select f1 from foo) for update;
                                           QUERY PLAN                                          
 ----------------------------------------------------------------------------------------------
  LockRows
-   Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.*
+   Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid, foo.ctid, foo.*, foo.tableoid
    ->  Hash Join
-         Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.*
+         Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid, foo.ctid, foo.*, foo.tableoid
          Hash Cond: (bar.f1 = foo.f1)
          ->  Append
                ->  Seq Scan on public.bar
-                     Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*
+                     Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid
                ->  Foreign Scan on public.bar2
-                     Output: bar2.f1, bar2.f2, bar2.ctid, bar2.tableoid, bar2.*
+                     Output: bar2.f1, bar2.f2, bar2.ctid, bar2.*, bar2.tableoid
                      Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
          ->  Hash
-               Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+               Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                ->  HashAggregate
-                     Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+                     Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                      Group Key: foo.f1
                      ->  Append
                            ->  Seq Scan on public.foo
-                                 Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+                                 Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                            ->  Foreign Scan on public.foo2
-                                 Output: foo2.ctid, foo2.tableoid, foo2.*, foo2.f1
+                                 Output: foo2.ctid, foo2.*, foo2.tableoid, foo2.f1
                                  Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1
 (22 rows)
 
@@ -3230,26 +3230,26 @@ select * from bar where f1 in (select f1 from foo) for share;
                                           QUERY PLAN                                          
 ----------------------------------------------------------------------------------------------
  LockRows
-   Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.*
+   Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid, foo.ctid, foo.*, foo.tableoid
    ->  Hash Join
-         Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.*
+         Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid, foo.ctid, foo.*, foo.tableoid
          Hash Cond: (bar.f1 = foo.f1)
          ->  Append
                ->  Seq Scan on public.bar
-                     Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*
+                     Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid
                ->  Foreign Scan on public.bar2
-                     Output: bar2.f1, bar2.f2, bar2.ctid, bar2.tableoid, 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: foo.ctid, foo.tableoid, foo.*, foo.f1
+               Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                ->  HashAggregate
-                     Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+                     Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                      Group Key: foo.f1
                      ->  Append
                            ->  Seq Scan on public.foo
-                                 Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+                                 Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                            ->  Foreign Scan on public.foo2
-                                 Output: foo2.ctid, foo2.tableoid, foo2.*, foo2.f1
+                                 Output: foo2.ctid, foo2.*, foo2.tableoid, foo2.f1
                                  Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1
 (22 rows)
 
@@ -3272,37 +3272,37 @@ update bar set f2 = f2 + 100 where f1 in (select f1 from foo);
    Foreign Update on public.bar2
      Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1
    ->  Hash Join
-         Output: bar.f1, (bar.f2 + 100), bar.ctid, foo.ctid, foo.tableoid, foo.*
+         Output: bar.f1, (bar.f2 + 100), bar.ctid, foo.ctid, foo.*, foo.tableoid
          Hash Cond: (bar.f1 = foo.f1)
          ->  Seq Scan on public.bar
                Output: bar.f1, bar.f2, bar.ctid
          ->  Hash
-               Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+               Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                ->  HashAggregate
-                     Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+                     Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                      Group Key: foo.f1
                      ->  Append
                            ->  Seq Scan on public.foo
-                                 Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+                                 Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                            ->  Foreign Scan on public.foo2
-                                 Output: foo2.ctid, foo2.tableoid, foo2.*, foo2.f1
+                                 Output: foo2.ctid, foo2.*, foo2.tableoid, foo2.f1
                                  Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1
    ->  Hash Join
-         Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, foo.ctid, foo.tableoid, foo.*
+         Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, foo.ctid, foo.*, foo.tableoid
          Hash Cond: (bar2.f1 = foo.f1)
          ->  Foreign Scan on public.bar2
                Output: bar2.f1, bar2.f2, bar2.f3, bar2.ctid
                Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
          ->  Hash
-               Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+               Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                ->  HashAggregate
-                     Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+                     Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                      Group Key: foo.f1
                      ->  Append
                            ->  Seq Scan on public.foo
-                                 Output: foo.ctid, foo.tableoid, foo.*, foo.f1
+                                 Output: foo.ctid, foo.*, foo.tableoid, foo.f1
                            ->  Foreign Scan on public.foo2
-                                 Output: foo2.ctid, foo2.tableoid, foo2.*, foo2.f1
+                                 Output: foo2.ctid, foo2.*, foo2.tableoid, foo2.f1
                                  Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1
 (37 rows)
 
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index 08e7c44..580c846 100644
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -107,23 +107,6 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
 								  pstrdup(resname),
 								  true);
 			tlist = lappend(tlist, tle);
-
-			/* if parent of inheritance tree, need the tableoid too */
-			if (rc->isParent)
-			{
-				var = makeVar(rc->rti,
-							  TableOidAttributeNumber,
-							  OIDOID,
-							  -1,
-							  InvalidOid,
-							  0);
-				snprintf(resname, sizeof(resname), "tableoid%u", rc->rowmarkId);
-				tle = makeTargetEntry((Expr *) var,
-									  list_length(tlist) + 1,
-									  pstrdup(resname),
-									  true);
-				tlist = lappend(tlist, tle);
-			}
 		}
 		if (rc->allMarkTypes & (1 << ROW_MARK_COPY))
 		{
@@ -139,6 +122,23 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
 								  true);
 			tlist = lappend(tlist, tle);
 		}
+
+		/* If parent of inheritance tree, always fetch the tableoid too. */
+		if (rc->isParent)
+		{
+			var = makeVar(rc->rti,
+						  TableOidAttributeNumber,
+						  OIDOID,
+						  -1,
+						  InvalidOid,
+						  0);
+			snprintf(resname, sizeof(resname), "tableoid%u", rc->rowmarkId);
+			tle = makeTargetEntry((Expr *) var,
+								  list_length(tlist) + 1,
+								  pstrdup(resname),
+								  true);
+			tlist = lappend(tlist, tle);
+		}
 	}
 
 	/*
-- 
1.9.1

Alvaro Herrera

alvherre@2ndquadrant.com

over 10 years ago

In reply to: Stephen Frost (#1)

Re: preprocess_targetlist and inheiritance

Stephen Frost wrote:

Tom, all,

Looks like preprocess_targetlist() should have been adjusted with the
changes to ExecBuildAuxRowMark() to support foreign tables being part
of inheritance trees (cb1ca4d800621dcae67ca6c799006de99fa4f0a5) to
also include the tableoid regardless of the rowMark type, if the
relation is the parent of an inheritance tree.

Uh, this patch was already submitted separately:
/messages/by-id/552CF0B6.8010006@lab.ntt.co.jp

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Stephen Frost

sfrost@snowman.net

over 10 years ago

In reply to: Alvaro Herrera (#2)

Re: preprocess_targetlist and inheiritance

Alvaro,

On Tuesday, April 21, 2015, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Stephen Frost wrote:

Tom, all,

Looks like preprocess_targetlist() should have been adjusted with the
changes to ExecBuildAuxRowMark() to support foreign tables being part
of inheritance trees (cb1ca4d800621dcae67ca6c799006de99fa4f0a5) to
also include the tableoid regardless of the rowMark type, if the
relation is the parent of an inheritance tree.

Uh, this patch was already submitted separately:
/messages/by-id/552CF0B6.8010006@lab.ntt.co.jp

Oh, excellent. Looks like the discussion agrees that it makes sense to do
and the RLS case didn't involve any foreign tables yet still ran into the
issue.

Is that correct or am I missing something?

Apologies, on my mobile currently.

Thanks!

Stephen

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 10 years ago

In reply to: Stephen Frost (#3)

Re: preprocess_targetlist and inheiritance

On 2015/04/22 0:43, Stephen Frost wrote:

On Tuesday, April 21, 2015, Alvaro Herrera <alvherre@2ndquadrant.com
<mailto:alvherre@2ndquadrant.com>> wrote:

Stephen Frost wrote:

Tom, all,

Looks like preprocess_targetlist() should have been adjusted

with the

changes to ExecBuildAuxRowMark() to support foreign tables

being part

of inheritance trees (cb1ca4d800621dcae67ca6c799006de99fa4f0a5) to
also include the tableoid regardless of the rowMark type, if the
relation is the parent of an inheritance tree.

Uh, this patch was already submitted separately:
/messages/by-id/552CF0B6.8010006@lab.ntt.co.jp

Oh, excellent. Looks like the discussion agrees that it makes sense to
do and the RLS case didn't involve any foreign tables yet still ran into
the issue.

IIUC, I think the issue would be raised when performing SELECT FOR
UPDATE/SHARE on an inheritance set the parents and childs of which are
all foreign tables, or when performing UPDATE/DELETE involving such an
inheritance set as a source table. Probably, I think your new
RLS-with-inheritance regression tests include such a query.

Best regards,
Etsuro Fujita

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers