Обсуждение: fdw_scan_tlist for foreign table scans breaks EPQ testing, doesn't it?
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
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>
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
> -----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
> -----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
> 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