Обсуждение: 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

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