Re: foreign join error "variable not found in subplan target list"

Hi.

Using the following patch I'm able to get
"ERROR: variable not found in subplan target list"
on foreign join pushdown for update returning.
It seems that we generate Result node which has 7 vars in targetlist
over Sort plan node, which has only 6 vars in targetlist.

So far not sure what causes this.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Hi Alexander,

On Tue, Aug 9, 2022 at 12:26 AM Alexander Pyhalov
<a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
> Using the following patch I'm able to get
> "ERROR: variable not found in subplan target list"
> on foreign join pushdown for update returning.

Reproduced here. Will look into this.

Thanks for the report!

Best regards,
Etsuro Fujita

On Tue, Aug 9, 2022 at 9:59 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
wrote:

> On Tue, Aug 9, 2022 at 12:26 AM Alexander Pyhalov
> <a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
> > Using the following patch I'm able to get
> > "ERROR: variable not found in subplan target list"
> > on foreign join pushdown for update returning.
>
> Reproduced here. Will look into this.
>
> Thanks for the report!

A rough look shows to me that the part of plan that causes problem looks
like:

-> Result
Output: ft2.ctid, ft2.*, ft4.*, ft4.c1, ft2.c2
-> Sort
Output: ft2.ctid, ft2.*, ft4.*, ft4.c1
Sort Key: ft4.c1

Note that for node 'Result', one of its target entries, 'ft2.c2', does
not appear in the target list of its subplan, i.e. node 'Sort'.

I think something goes wrong in postgresGetForeignPlan() when we build
the list of columns to be fetched from the foreign server, and fix the
subplan's tlist with that, where we would include in columns specified
in local conditions. In this case the local condition is 'ft2.c2 ===
ft4.c1', and that's how we have entry 'ft2.c2' in Result's target list.

Thanks
Richard

Richard Guo писал 2022-08-09 06:51:
> On Tue, Aug 9, 2022 at 9:59 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> wrote:
>
>> On Tue, Aug 9, 2022 at 12:26 AM Alexander Pyhalov
>> <a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
>>> Using the following patch I'm able to get
>>> "ERROR: variable not found in subplan target list"
>>> on foreign join pushdown for update returning.
>>
>> Reproduced here. Will look into this.
>>
>> Thanks for the report!
>
> A rough look shows to me that the part of plan that causes problem
> looks
> like:
>
> -> Result
> Output: ft2.ctid, ft2.*, ft4.*, ft4.c1, ft2.c2
> -> Sort
> Output: ft2.ctid, ft2.*, ft4.*, ft4.c1
> Sort Key: ft4.c1
>
> Note that for node 'Result', one of its target entries, 'ft2.c2', does
> not appear in the target list of its subplan, i.e. node 'Sort'.
>
> I think something goes wrong in postgresGetForeignPlan() when we build
> the list of columns to be fetched from the foreign server, and fix the
> subplan's tlist with that, where we would include in columns specified
> in local conditions. In this case the local condition is 'ft2.c2 ===
> ft4.c1', and that's how we have entry 'ft2.c2' in Result's target
> list.
>
> Thanks
> Richard

Hi.
The issue seems to appear due to the fact that Sort path doesn't provide
all vars, needed by local_conds.
What if we check for this condition specifically?

--
Best regards,
Alexander Pyhalov,
Postgres Professional

On Tue, Aug 9, 2022 at 7:54 PM Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
wrote:

> Richard Guo писал 2022-08-09 06:51:
> > On Tue, Aug 9, 2022 at 9:59 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> > wrote:
> >
> >> On Tue, Aug 9, 2022 at 12:26 AM Alexander Pyhalov
> >> <a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
> >>> Using the following patch I'm able to get
> >>> "ERROR: variable not found in subplan target list"
> >>> on foreign join pushdown for update returning.
> >>
> >> Reproduced here. Will look into this.
> >>
> >> Thanks for the report!
> >
> > A rough look shows to me that the part of plan that causes problem
> > looks
> > like:
> >
> > -> Result
> > Output: ft2.ctid, ft2.*, ft4.*, ft4.c1, ft2.c2
> > -> Sort
> > Output: ft2.ctid, ft2.*, ft4.*, ft4.c1
> > Sort Key: ft4.c1
> >
> > Note that for node 'Result', one of its target entries, 'ft2.c2', does
> > not appear in the target list of its subplan, i.e. node 'Sort'.
> >
> > I think something goes wrong in postgresGetForeignPlan() when we build
> > the list of columns to be fetched from the foreign server, and fix the
> > subplan's tlist with that, where we would include in columns specified
> > in local conditions. In this case the local condition is 'ft2.c2 ===
> > ft4.c1', and that's how we have entry 'ft2.c2' in Result's target
> > list.
> >
> > Thanks
> > Richard
>
> Hi.
> The issue seems to appear due to the fact that Sort path doesn't provide
> all vars, needed by local_conds.
> What if we check for this condition specifically?

I think this change may make us miss some EPQ path with pathkeys, which
seems not good. Also, Sort path would always have the same pathtarget as
its subpath. So if we really want to do this, we can do it earlier in
the caller of add_paths_with_pathkeys_for_rel.

Currently the outer_plan used in postgresGetForeignPlan() can only be
'Join' or 'Sort + Join'. I'm wondering whether we can take this
knowledge into consideration when we fix the outer_plan's tlist, to also
fix the Join's tlist if it is below the Sort node.

Thanks
Richard

On Wed, Aug 10, 2022 at 10:15 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:

> Currently the outer_plan used in postgresGetForeignPlan() can only be
> 'Join' or 'Sort + Join'. I'm wondering whether we can take this
> knowledge into consideration when we fix the outer_plan's tlist, to also
> fix the Join's tlist if it is below the Sort node.
>

Alternatively, how about we include in the EPQ path's pathtarget the
columns required for evaluating the local conditions when we consider
EPQ paths with pathkeys? Something like attached.

Thanks
Richard

Richard Guo писал 2022-08-10 08:28:
> On Wed, Aug 10, 2022 at 10:15 AM Richard Guo <guofenglinux(at)gmail(dot)com>
> wrote:
>
>> Currently the outer_plan used in postgresGetForeignPlan() can only
>> be
>> 'Join' or 'Sort + Join'. I'm wondering whether we can take this
>> knowledge into consideration when we fix the outer_plan's tlist, to
>> also
>> fix the Join's tlist if it is below the Sort node.
>
> Alternatively, how about we include in the EPQ path's pathtarget
> thecolumns required for evaluating the local conditions when we
> consider
> EPQ paths with pathkeys? Something like attached.
>
> Thanks
> Richard

Hi.
Why are we sure that epq_path can provide all vars from restrictinfo?
--
Best regards,
Alexander Pyhalov,
Postgres Professional

On Wed, Aug 10, 2022 at 3:06 PM Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
wrote:

> Richard Guo писал 2022-08-10 08:28:
> > On Wed, Aug 10, 2022 at 10:15 AM Richard Guo <guofenglinux(at)gmail(dot)com>
> > wrote:
> >
> >> Currently the outer_plan used in postgresGetForeignPlan() can only
> >> be
> >> 'Join' or 'Sort + Join'. I'm wondering whether we can take this
> >> knowledge into consideration when we fix the outer_plan's tlist, to
> >> also
> >> fix the Join's tlist if it is below the Sort node.
> >
> > Alternatively, how about we include in the EPQ path's pathtarget
> > thecolumns required for evaluating the local conditions when we
> > consider
> > EPQ paths with pathkeys? Something like attached.
> >
> > Thanks
> > Richard
>
> Hi.
> Why are we sure that epq_path can provide all vars from restrictinfo?

The local conditions come from the joinrel's restrictlist, which
contains all the clauses that syntactically belong at the join level. So
I think the join path for EPQ checks should be able to provide all the
exprs needed by local_conds.

Thanks
Richard

Richard Guo писал 2022-08-10 11:36:
> On Wed, Aug 10, 2022 at 3:06 PM Alexander Pyhalov
> <a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
>
>> Richard Guo писал 2022-08-10 08:28:
>>> On Wed, Aug 10, 2022 at 10:15 AM Richard Guo
>> <guofenglinux(at)gmail(dot)com>
>>> wrote:
>>>
>>>> Currently the outer_plan used in postgresGetForeignPlan() can
>> only
>>>> be
>>>> 'Join' or 'Sort + Join'. I'm wondering whether we can take this
>>>> knowledge into consideration when we fix the outer_plan's tlist,
>> to
>>>> also
>>>> fix the Join's tlist if it is below the Sort node.
>>>
>>> Alternatively, how about we include in the EPQ path's pathtarget
>>> thecolumns required for evaluating the local conditions when we
>>> consider
>>> EPQ paths with pathkeys? Something like attached.
>>>
>>> Thanks
>>> Richard
>>
>> Hi.
>> Why are we sure that epq_path can provide all vars from
>> restrictinfo?
>
> The local conditions come from the joinrel's restrictlist, which
> contains all the clauses that syntactically belong at the join level.
> So
> I think the join path for EPQ checks should be able to provide all the
> exprs needed by local_conds.
>
> Thanks
> Richard

OK. It looks good to me. The only thing which surprised me that in test
case we see unnecessary sort in remote query. However, it's explained by
selected costs and STD_FUZZ_FACTOR, so that sorted path has essentially
the same cost as non-sorted one according to
compare_path_costs_fuzzily().

EXPLAIN (verbose, costs off)
UPDATE ft2 SET c3 = 'baz'
FROM ft4 INNER JOIN ft5 ON (ft4.c1 = ft5.c1)
WHERE ft2.c1 > 2000 AND ft2.c2 === ft4.c1
RETURNING ft2.*, ft4.*, ft5.*;


QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Update on public.ft2
Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7,
ft2.c8, ft4.c1, ft4.c2, ft4.c3, ft5.c1, ft5.c2, ft5.c3
Remote SQL: UPDATE "S 1"."T 1" SET c3 = $2 WHERE ctid = $1 RETURNING
"C 1", c2, c3, c4, c5, c6, c7, c8
-> Hash Join
Output: 'baz'::text, ft2.ctid, ft2.*, ft4.*, ft5.*, ft4.c1,
ft4.c2, ft4.c3, ft5.c1, ft5.c2, ft5.c3
Hash Cond: (ft5.c1 = ft4.c1)
-> Foreign Scan on public.ft5
Output: ft5.*, ft5.c1, ft5.c2, ft5.c3
Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
-> Hash
Output: ft2.ctid, ft2.*, ft4.*, ft4.c1, ft4.c2, ft4.c3,
ft2.c2
-> Foreign Scan
Output: ft2.ctid, ft2.*, ft4.*, ft4.c1, ft4.c2,
ft4.c3, ft2.c2
Filter: (ft2.c2 === ft4.c1)
Relations: (public.ft2) INNER JOIN (public.ft4)
Remote SQL: SELECT r1.ctid, CASE WHEN (r1.*)::text
IS NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7,
r1.c8) END, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2,
r2.c3) END, r2.c1, r2.c2, r2.c3, r1.c2 FROM ("S 1"."T 1" r1 INNER JOIN
"S 1"."T 3" r2 ON (((r1."C 1" > 2000)))) ORDER BY r2.c1 ASC NULLS LAST
FOR UPDATE OF r1
-> Sort
Output: ft2.ctid, ft2.*, ft4.*, ft4.c1,
ft4.c2, ft4.c3, ft2.c2
Sort Key: ft4.c1
-> Nested Loop
Output: ft2.ctid, ft2.*, ft4.*, ft4.c1,
ft4.c2, ft4.c3, ft2.c2
Join Filter: (ft2.c2 === ft4.c1)
-> Foreign Scan on public.ft2
Output: ft2.ctid, ft2.*, ft2.c2
Remote SQL: SELECT "C 1", c2, c3,
c4, c5, c6, c7, c8, ctid FROM "S 1"."T 1" WHERE (("C 1" > 2000)) FOR
UPDATE
-> Foreign Scan on public.ft4
Output: ft4.*, ft4.c1, ft4.c2,
ft4.c3
Remote SQL: SELECT c1, c2, c3
FROM "S 1"."T 3"

--
Best regards,
Alexander Pyhalov,
Postgres Professional

On Wed, Aug 10, 2022 at 11:17 PM Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
wrote:

> OK. It looks good to me. The only thing which surprised me that in test
> case we see unnecessary sort in remote query. However, it's explained by
> selected costs and STD_FUZZ_FACTOR, so that sorted path has essentially
> the same cost as non-sorted one according to
> compare_path_costs_fuzzily().

Yeah, and meanwhile the sorted path is considered to have better
pathkeys than the non-sorted one. So it wins in add_path().

Thanks
Richard

Hi Richard,

On Wed, Aug 10, 2022 at 2:28 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Wed, Aug 10, 2022 at 10:15 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>> Currently the outer_plan used in postgresGetForeignPlan() can only be
>> 'Join' or 'Sort + Join'. I'm wondering whether we can take this
>> knowledge into consideration when we fix the outer_plan's tlist, to also
>> fix the Join's tlist if it is below the Sort node.

> Alternatively, how about we include in the EPQ path's pathtarget the
> columns required for evaluating the local conditions when we consider
> EPQ paths with pathkeys? Something like attached.

Thanks for the patch! I reviewed the patch. I think the patch goes
in the right direction.

+ if (epq_path != NULL && useful_pathkeys_list != NIL)
+ {
+ /* Include columns required for evaluating the local conditions */
+ foreach(lc, fpinfo->local_conds)
+ {
+ RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
+
+ add_new_columns_to_pathtarget(epq_path->pathtarget,
+ pull_var_clause((Node *)
rinfo->clause,
+ PVC_RECURSE_PLACEHOLDERS));
+ }
+ }

* I think we should avoid modifying the pathtarget, because it would
be the default pathtarget, which other paths might reference. I think
it’s safe to use a copied pathtarget, like the attached.

* I think this issue occurs also when there are PlaceHolderVars in the
relation’s reltarget. Here is an example:

EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM local_tbl LEFT JOIN (SELECT ft1.*, COALESCE(ft1.c3 ||
ft2.c3, 'foobar') FROM ft1 INNER JOIN ft2 ON (ft1.c1 = ft2.c1 AND
ft1.c1 < 100)) ss ON (local_tbl.c1 = ss.c1) ORDER BY local_tbl.c1 FOR
UPDATE OF local_tbl;
ERROR: variable not found in subplan target list

where local_tbl, ft1, and ft2 are local/foreign tables defined as in
postgres_fdw.sql. To fix, I modified the patch so that we add to the
pathtarget not only columns required for evaluating the local
conditions but columns required for evaluating the PlaceHoderVars.

* The test case reported in this thread produces the planner error
only in v14 and later, but I think this issue exists since v9.6. So I
created/added new test cases, including the above one, that would
otherwise produce the error even in previous versions.

Best regards,
Etsuro Fujita

On Sun, Aug 21, 2022 at 8:31 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
wrote:

> * I think we should avoid modifying the pathtarget, because it would
> be the default pathtarget, which other paths might reference. I think
> it’s safe to use a copied pathtarget, like the attached.

Yeah, that's right. The EPQ path is a shallow copy of the original. A
copied pathtarget should be saner here.

>
> * I think this issue occurs also when there are PlaceHolderVars in the
> relation’s reltarget. Here is an example:
>
> EXPLAIN (VERBOSE, COSTS OFF)
> SELECT * FROM local_tbl LEFT JOIN (SELECT ft1.*, COALESCE(ft1.c3 ||
> ft2.c3, 'foobar') FROM ft1 INNER JOIN ft2 ON (ft1.c1 = ft2.c1 AND
> ft1.c1 < 100)) ss ON (local_tbl.c1 = ss.c1) ORDER BY local_tbl.c1 FOR
> UPDATE OF local_tbl;
> ERROR: variable not found in subplan target list
>
> where local_tbl, ft1, and ft2 are local/foreign tables defined as in
> postgres_fdw.sql. To fix, I modified the patch so that we add to the
> pathtarget not only columns required for evaluating the local
> conditions but columns required for evaluating the PlaceHoderVars.

Correct. Good catch! I can reproduce this error regarding PHVs in master
and verify that this change can fix it. It makes the logic of adjusting
the pathtarget for the epq_path in add_paths_with_pathkeys_for_rel be
more consistent with the logic in build_tlist_to_deparse, which is good.

>
> * The test case reported in this thread produces the planner error
> only in v14 and later, but I think this issue exists since v9.6. So I
> created/added new test cases, including the above one, that would
> otherwise produce the error even in previous versions.

+1 for the tests.

Thanks
Richard

On Mon, Aug 22, 2022 at 12:29 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Sun, Aug 21, 2022 at 8:31 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
>> * I think we should avoid modifying the pathtarget, because it would
>> be the default pathtarget, which other paths might reference. I think
>> it’s safe to use a copied pathtarget, like the attached.

> Yeah, that's right. The EPQ path is a shallow copy of the original. A
> copied pathtarget should be saner here.

One thing I noticed while re-reading the patch is that we should use
create_projection_path() here, to avoid modifying the epq_path in
place, as it is already used for an unsorted join-pushdown path before
we get here. So I modified the patch as such. Updated patch
attached.

Thanks for reviewing!

Best regards,
Etsuro Fujita

On Mon, Aug 29, 2022 at 2:17 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
wrote:

> One thing I noticed while re-reading the patch is that we should use
> create_projection_path() here, to avoid modifying the epq_path in
> place, as it is already used for an unsorted join-pushdown path before
> we get here. So I modified the patch as such. Updated patch
> attached.

The new change looks reasonable to me as there is other ref to this
epq_path.

I'm looking again on how we adjust the PathTarget, and I think we may
need to update the cost and width fields if there are any new columns
added. Maybe we can leverage set_pathtarget_cost_width for that.

Thanks
Richard

On Mon, Aug 29, 2022 at 5:45 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:

>
> On Mon, Aug 29, 2022 at 2:17 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> wrote:
>
>> One thing I noticed while re-reading the patch is that we should use
>> create_projection_path() here, to avoid modifying the epq_path in
>> place, as it is already used for an unsorted join-pushdown path before
>> we get here. So I modified the patch as such. Updated patch
>> attached.
>
>
> The new change looks reasonable to me as there is other ref to this
> epq_path.
>
> I'm looking again on how we adjust the PathTarget, and I think we may
> need to update the cost and width fields if there are any new columns
> added. Maybe we can leverage set_pathtarget_cost_width for that.
>

To be concrete, I mean something like this:

--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5808,6 +5808,10 @@ add_paths_with_pathkeys_for_rel(PlannerInfo *root,
RelOptInfo *rel,

PVC_RECURSE_PLACEHOLDERS));
}

+ /* Update the cost and width fields if we have added any
new columns. */
+ if (!equal(epq_path->pathtarget->exprs, target->exprs))
+ set_pathtarget_cost_width(root, target);
+
/*
* The passed-in EPQ path is a join path, so it is
projection-capable,
* but we use create_projection_path() here, so as to avoid
modifying

Thanks
Richard

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> To be concrete, I mean something like this:

> + /* Update the cost and width fields if we have added any
> new columns. */
> + if (!equal(epq_path->pathtarget->exprs, target->exprs))
> + set_pathtarget_cost_width(root, target);

Wouldn't a list_length() comparison be sufficient? equal() seems
like overkill.

regards, tom lane

On Tue, Aug 30, 2022 at 10:04 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> > To be concrete, I mean something like this:
>
> > + /* Update the cost and width fields if we have added any
> > new columns. */
> > + if (!equal(epq_path->pathtarget->exprs, target->exprs))
> > + set_pathtarget_cost_width(root, target);
>
> Wouldn't a list_length() comparison be sufficient? equal() seems
> like overkill.

Yeah, list_length comparison would do, as we only append new columns
here. Thanks for the suggestion!

Thanks
Richard

On Mon, Aug 29, 2022 at 6:46 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> I'm looking again on how we adjust the PathTarget, and I think we may
> need to update the cost and width fields if there are any new columns
> added. Maybe we can leverage set_pathtarget_cost_width for that.

I'm not really sure we really need to make the estimates accurate,
because 1) the resulting EPQ plan is used only for rechecks, so the
estimates would not be that important IMO, and 2) in fact, we don't
adjust the estimates of a given EPQ plan in postgresGetForeignPlan()
even when replacing the plan's tlist and/or removing local conditions
from the plan. Is it worth expending the cycles here?

Sorry for the late reply.

Best regards,
Etsuro Fujita

On Tue, Aug 30, 2022 at 3:50 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
wrote:

> On Mon, Aug 29, 2022 at 6:46 PM Richard Guo <guofenglinux(at)gmail(dot)com>
> wrote:
> > I'm looking again on how we adjust the PathTarget, and I think we may
> > need to update the cost and width fields if there are any new columns
> > added. Maybe we can leverage set_pathtarget_cost_width for that.
>
> I'm not really sure we really need to make the estimates accurate,
> because 1) the resulting EPQ plan is used only for rechecks, so the
> estimates would not be that important IMO, and 2) in fact, we don't
> adjust the estimates of a given EPQ plan in postgresGetForeignPlan()
> even when replacing the plan's tlist and/or removing local conditions
> from the plan. Is it worth expending the cycles here?

Hmm, yes, the EPQ path does not need to compete with others in add_path,
so its cost does not matter too much. And the comment just above
GetExistingLocalJoinPath says:

* Since the plan created using this path will presumably only be used to
* execute EPQ checks, efficiency of the path is not a concern.

But still I feel it's not a good practice to not update the cost and
width fields after calling add_new_columns_to_pathtarget(). How about we
add some comments here explaining why we do not need to adjust the
estimates for the EPQ path?

Thanks
Richard

On Wed, Aug 31, 2022 at 6:27 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Tue, Aug 30, 2022 at 3:50 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
>> On Mon, Aug 29, 2022 at 6:46 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>> > I'm looking again on how we adjust the PathTarget, and I think we may
>> > need to update the cost and width fields if there are any new columns
>> > added. Maybe we can leverage set_pathtarget_cost_width for that.
>>
>> I'm not really sure we really need to make the estimates accurate,
>> because 1) the resulting EPQ plan is used only for rechecks, so the
>> estimates would not be that important IMO, and 2) in fact, we don't
>> adjust the estimates of a given EPQ plan in postgresGetForeignPlan()
>> even when replacing the plan's tlist and/or removing local conditions
>> from the plan. Is it worth expending the cycles here?

> Hmm, yes, the EPQ path does not need to compete with others in add_path,
> so its cost does not matter too much. And the comment just above
> GetExistingLocalJoinPath says:
>
> * Since the plan created using this path will presumably only be used to
> * execute EPQ checks, efficiency of the path is not a concern.
>
> But still I feel it's not a good practice to not update the cost and
> width fields after calling add_new_columns_to_pathtarget(). How about we
> add some comments here explaining why we do not need to adjust the
> estimates for the EPQ path?

I agree with you on that point. I’ll update the patch as such in the
next version.

Best regards,
Etsuro Fujita

On Wed, Aug 31, 2022 at 6:52 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> On Wed, Aug 31, 2022 at 6:27 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> > Hmm, yes, the EPQ path does not need to compete with others in add_path,
> > so its cost does not matter too much. And the comment just above
> > GetExistingLocalJoinPath says:
> >
> > * Since the plan created using this path will presumably only be used to
> > * execute EPQ checks, efficiency of the path is not a concern.
> >
> > But still I feel it's not a good practice to not update the cost and
> > width fields after calling add_new_columns_to_pathtarget(). How about we
> > add some comments here explaining why we do not need to adjust the
> > estimates for the EPQ path?
>
> I agree with you on that point. I’ll update the patch as such in the
> next version.

Here is an updated patch for that. Other changes:

* I modified the patch so that we adjust the tlist of the EPQ path if
necessary, using the idea discussed upthread.
* I tweaked other comments a little bit.

Best regards,
Etsuro Fujita

On Mon, Sep 12, 2022 at 12:19 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
wrote:

> On Wed, Aug 31, 2022 at 6:52 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> wrote:
> > On Wed, Aug 31, 2022 at 6:27 PM Richard Guo <guofenglinux(at)gmail(dot)com>
> wrote:
> > > Hmm, yes, the EPQ path does not need to compete with others in
> add_path,
> > > so its cost does not matter too much. And the comment just above
> > > GetExistingLocalJoinPath says:
> > >
> > > * Since the plan created using this path will presumably only be used
> to
> > > * execute EPQ checks, efficiency of the path is not a concern.
> > >
> > > But still I feel it's not a good practice to not update the cost and
> > > width fields after calling add_new_columns_to_pathtarget(). How about
> we
> > > add some comments here explaining why we do not need to adjust the
> > > estimates for the EPQ path?
> >
> > I agree with you on that point. I’ll update the patch as such in the
> > next version.
>
> Here is an updated patch for that. Other changes:
>
> * I modified the patch so that we adjust the tlist of the EPQ path if
> necessary, using the idea discussed upthread.
> * I tweaked other comments a little bit.

+1. The new patch looks good to me.

Thanks
Richard

On Tue, Sep 13, 2022 at 11:28 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Mon, Sep 12, 2022 at 12:19 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
>> Here is an updated patch for that.

> +1. The new patch looks good to me.

Pushed after tweaking comments a little bit further.

Thanks!

Best regards,
Etsuro Fujita