Thread: fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it?

fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it?

From
Etsuro Fujita
Date:
Hi,

The latest foreign-join pushdown patch allows fdw_scan_tlist to be set
to a targetlist even for simple foreign table scans.  However, since I
think we assume that the test tuple of a foreign table for an EPQ
testing, whether it may be copied from the whole-row var or returned by
the RefetchForeignRow routine, has the rowtype declared for the foreign
table, ISTM that EPQ testing doesn't work properly in such a case since
that the targetlist and qual are adjusted to reference fdw_scan_tlist in
such a case.  Maybe I'm missing something though.

I don't understand custom scans/joins exactly, but I have a similar
concern for the simple-custom-scan case too.

Best regards,
Etsuro Fujita



Re: fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it?

From
Kouhei Kaigai
Date:
Fujita-san,

Sorry for my late response.

> The latest foreign-join pushdown patch allows fdw_scan_tlist to be set
> to a targetlist even for simple foreign table scans.  However, since I
> think we assume that the test tuple of a foreign table for an EPQ
> testing, whether it may be copied from the whole-row var or returned by
> the RefetchForeignRow routine, has the rowtype declared for the foreign
> table, ISTM that EPQ testing doesn't work properly in such a case since
> that the targetlist and qual are adjusted to reference fdw_scan_tlist in
> such a case.  Maybe I'm missing something though.
>
Let me confirm step-by-step.
For EPQ testing, whole-row-reference or RefetchForeignRow pulls a record
with row-type compatible to the base foreign table. Then, this record
is stored in the es_epqTuple[] indexed by the base relation.

According to the previous discussion, I expect these tuples are re-checked
by built-in execution plan, but equivalent to the sub-plan entirely pushed
out to the remote side.
Do we see the same assumption?

If so, next step is enhancement of ExecScanFetch() to run the alternative
built-in plans towards each es_epqTuple[] records, if given scanrelid==0.
In this case, expression nodes adjusted to fdw_scan_tlist never called,
so it should not lead any problems...?

> I don't understand custom scans/joins exactly, but I have a similar
> concern for the simple-custom-scan case too.
>
In case of custom scan/join, it fetches a record using heap_fetch()
identified by ctid, and saved to es_epqTuple[].
Then, EvalPlanQual() walks down the plan-tree. Once it appears a node
of custom-join (scanrelid==0), it shall call the equivalent alternatives
if possible, or calls ExecProcNode() towards the underlying nodes then
re-construct its result according to the custom_scan_tlist definition.

It does not look to me problematic.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>



Re: fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it?

From
Etsuro Fujita
Date:
Hi KaiGai-san,

On 2015/07/22 16:44, Kouhei Kaigai wrote:
>> The latest foreign-join pushdown patch allows fdw_scan_tlist to be set
>> to a targetlist even for simple foreign table scans.  However, since I
>> think we assume that the test tuple of a foreign table for an EPQ
>> testing, whether it may be copied from the whole-row var or returned by
>> the RefetchForeignRow routine, has the rowtype declared for the foreign
>> table, ISTM that EPQ testing doesn't work properly in such a case since
>> that the targetlist and qual are adjusted to reference fdw_scan_tlist in
>> such a case.  Maybe I'm missing something though.
>>
> Let me confirm step-by-step.
> For EPQ testing, whole-row-reference or RefetchForeignRow pulls a record
> with row-type compatible to the base foreign table. Then, this record
> is stored in the es_epqTuple[] indexed by the base relation.
> 
> According to the previous discussion, I expect these tuples are re-checked
> by built-in execution plan, but equivalent to the sub-plan entirely pushed
> out to the remote side.
> Do we see the same assumption?

No, what I'm concerned about is the case when scanrelid > 0.

> If so, next step is enhancement of ExecScanFetch() to run the alternative
> built-in plans towards each es_epqTuple[] records, if given scanrelid==0.
> In this case, expression nodes adjusted to fdw_scan_tlist never called,
> so it should not lead any problems...?

When scanrelid = 0, I think we should run the alternative plans in
ExecScanFetch or somewhere, as you mentioned.

>> I don't understand custom scans/joins exactly, but I have a similar
>> concern for the simple-custom-scan case too.
>>
> In case of custom scan/join, it fetches a record using heap_fetch()
> identified by ctid, and saved to es_epqTuple[].
> Then, EvalPlanQual() walks down the plan-tree. Once it appears a node
> of custom-join (scanrelid==0), it shall call the equivalent alternatives
> if possible, or calls ExecProcNode() towards the underlying nodes then
> re-construct its result according to the custom_scan_tlist definition.
> 
> It does not look to me problematic.

Sorry, I don't understand what you mean.  Maybe I have to learn more
about custom scans/joins, but thanks for the explanation!

Best regards,
Etsuro Fujita



Re: fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it?

From
Kouhei Kaigai
Date:
> -----Original Message-----
> From: Etsuro Fujita [mailto:fujita.etsuro@lab.ntt.co.jp]
> Sent: Wednesday, July 22, 2015 7:05 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] fdw_scan_tlist for foreign table scans breaks EPQ testing,
> doesn't it?
>
> Hi KaiGai-san,
>
> On 2015/07/22 16:44, Kouhei Kaigai wrote:
> >> The latest foreign-join pushdown patch allows fdw_scan_tlist to be set
> >> to a targetlist even for simple foreign table scans.  However, since I
> >> think we assume that the test tuple of a foreign table for an EPQ
> >> testing, whether it may be copied from the whole-row var or returned by
> >> the RefetchForeignRow routine, has the rowtype declared for the foreign
> >> table, ISTM that EPQ testing doesn't work properly in such a case since
> >> that the targetlist and qual are adjusted to reference fdw_scan_tlist in
> >> such a case.  Maybe I'm missing something though.
> >>
> > Let me confirm step-by-step.
> > For EPQ testing, whole-row-reference or RefetchForeignRow pulls a record
> > with row-type compatible to the base foreign table. Then, this record
> > is stored in the es_epqTuple[] indexed by the base relation.
> >
> > According to the previous discussion, I expect these tuples are re-checked
> > by built-in execution plan, but equivalent to the sub-plan entirely pushed
> > out to the remote side.
> > Do we see the same assumption?
>
> No, what I'm concerned about is the case when scanrelid > 0.
>
Hmm. if scanrelid > 0, then fdw_scan_tlist should be NIL.
I want to put Assert(scanrelid==0 || fdw_scan_tlist == NIL) just after
the GetForeignPlan() in createplan.c.

I'm curious why you tried to put valid fdw_scan_tlist for scanrelid > 0.
It's unusual.

> > If so, next step is enhancement of ExecScanFetch() to run the alternative
> > built-in plans towards each es_epqTuple[] records, if given scanrelid==0.
> > In this case, expression nodes adjusted to fdw_scan_tlist never called,
> > so it should not lead any problems...?
>
> When scanrelid = 0, I think we should run the alternative plans in
> ExecScanFetch or somewhere, as you mentioned.
>
OK,

> >> I don't understand custom scans/joins exactly, but I have a similar
> >> concern for the simple-custom-scan case too.
> >>
> > In case of custom scan/join, it fetches a record using heap_fetch()
> > identified by ctid, and saved to es_epqTuple[].
> > Then, EvalPlanQual() walks down the plan-tree. Once it appears a node
> > of custom-join (scanrelid==0), it shall call the equivalent alternatives
> > if possible, or calls ExecProcNode() towards the underlying nodes then
> > re-construct its result according to the custom_scan_tlist definition.
> >
> > It does not look to me problematic.
>
> Sorry, I don't understand what you mean.  Maybe I have to learn more
> about custom scans/joins, but thanks for the explanation!
>
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>



On Wed, Jul 22, 2015 at 8:13 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
>> No, what I'm concerned about is the case when scanrelid > 0.
>>
> Hmm. if scanrelid > 0, then fdw_scan_tlist should be NIL.
> I want to put Assert(scanrelid==0 || fdw_scan_tlist == NIL) just after
> the GetForeignPlan() in createplan.c.
>
> I'm curious why you tried to put valid fdw_scan_tlist for scanrelid > 0.
> It's unusual.

Allowing that was part of the point of Tom Lane's commit
1a8a4e5cde2b7755e11bde2ea7897bd650622d3e.  See the second bullet
point, after the comma.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it?

From
Kouhei Kaigai
Date:
> -----Original Message-----
> From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas
> Sent: Wednesday, July 22, 2015 11:19 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Etsuro Fujita; pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] fdw_scan_tlist for foreign table scans breaks EPQ testing,
> doesn't it?
> 
> On Wed, Jul 22, 2015 at 8:13 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
> >> No, what I'm concerned about is the case when scanrelid > 0.
> >>
> > Hmm. if scanrelid > 0, then fdw_scan_tlist should be NIL.
> > I want to put Assert(scanrelid==0 || fdw_scan_tlist == NIL) just after
> > the GetForeignPlan() in createplan.c.
> >
> > I'm curious why you tried to put valid fdw_scan_tlist for scanrelid > 0.
> > It's unusual.
> 
> Allowing that was part of the point of Tom Lane's commit
> 1a8a4e5cde2b7755e11bde2ea7897bd650622d3e.  See the second bullet
> point, after the comma.
>
Indeed, this commit allows ForeignScan to have fdw_scan_tlist, even if
scanrelid > 0, however, I'm uncertain about its reason/intention.
Does it a preparation for the upcoming target-list-pushdown??

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>


On Wed, Jul 22, 2015 at 8:24 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
> Indeed, this commit allows ForeignScan to have fdw_scan_tlist, even if
> scanrelid > 0, however, I'm uncertain about its reason/intention.
> Does it a preparation for the upcoming target-list-pushdown??

I guess Tom would have to comment on whether it could be used for that
purpose.  I assume that omitting columns could be interesting for some
FDWs, if nothing else.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it?

From
Kouhei Kaigai
Date:
> On Wed, Jul 22, 2015 at 8:24 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
> > Indeed, this commit allows ForeignScan to have fdw_scan_tlist, even if
> > scanrelid > 0, however, I'm uncertain about its reason/intention.
> > Does it a preparation for the upcoming target-list-pushdown??
> 
> I guess Tom would have to comment on whether it could be used for that
> purpose.  I assume that omitting columns could be interesting for some
> FDWs, if nothing else.
>
Indeed. As current postgres_fdw doing, FDW driver puts dummy NULLs
on unreferenced columns for network optimization, however, it shall
become unnecessary if we can change definition of the expected
record-type of foreign-table. Its advantage is more human readable
remote query and less CPU cycle for projection.

A dark side is, as discussed in this thread, complexity of EvalPlanQual.
RefetchForeignRow() returns a tuple based on foreign table definition,
on the other hands, whole-row var points a tuple based on fdw_scan_tlist
if exists.
An alternative host-only plan-node and relevant expression will be
constructed towards the definition of base foreign-table. So, we need to
transform the tuple to the layout based on foreign table definition if
we allow fdw_scan_tlist with scanrelid > 0.

However, I'm skeptical whether this solution is valid for long term.
Once we support to push down expensive expression in target-list to
remote side, fdw_scan_tlist will contain expression node rather than
simple Var node. In this case, it is not obvious to reproduce a tuple
according to the foreign table definition from a record based on the
fdw_scan_tlist.

So, I'm inclined to prohibit to set fdw_scan_tlist/custom_scan_tlist
for actual scan node (scanrelid > 0), at present.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

On Thu, Jul 23, 2015 at 8:27 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
> A dark side is, as discussed in this thread, complexity of EvalPlanQual.
> RefetchForeignRow() returns a tuple based on foreign table definition,
> on the other hands, whole-row var points a tuple based on fdw_scan_tlist
> if exists.
> An alternative host-only plan-node and relevant expression will be
> constructed towards the definition of base foreign-table. So, we need to
> transform the tuple to the layout based on foreign table definition if
> we allow fdw_scan_tlist with scanrelid > 0.
>
> However, I'm skeptical whether this solution is valid for long term.
> Once we support to push down expensive expression in target-list to
> remote side, fdw_scan_tlist will contain expression node rather than
> simple Var node. In this case, it is not obvious to reproduce a tuple
> according to the foreign table definition from a record based on the
> fdw_scan_tlist.

I don't think we can realistically make a decision that pushing down
target list expressions to the remote side is forever off the table.

Is the problem here that it's not *possible* for an FDW to do the
right thing, or just that it might be difficult to code in practice?
I'm fuzzy on why this isn't just a matter of having
RefetchForeignRow() return a row with the correct tuple descriptor.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company