Thread: 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.pyhalov@postgrespro.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

Richard Guo писал 2022-08-09 06:51:
> On Tue, Aug 9, 2022 at 9:59 AM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> 
>> On Tue, Aug 9, 2022 at 12:26 AM Alexander Pyhalov
>> <a.pyhalov@postgrespro.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

Richard Guo писал 2022-08-10 08:28:
> On Wed, Aug 10, 2022 at 10:15 AM Richard Guo <guofenglinux@gmail.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

Richard Guo писал 2022-08-10 11:36:
> On Wed, Aug 10, 2022 at 3:06 PM Alexander Pyhalov
> <a.pyhalov@postgrespro.ru> wrote:
> 
>> Richard Guo писал 2022-08-10 08:28:
>>> On Wed, Aug 10, 2022 at 10:15 AM Richard Guo
>> <guofenglinux@gmail.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

Hi Richard,

On Wed, Aug 10, 2022 at 2:28 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Wed, Aug 10, 2022 at 10:15 AM Richard Guo <guofenglinux@gmail.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 Mon, Aug 22, 2022 at 12:29 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Sun, Aug 21, 2022 at 8:31 PM Etsuro Fujita <etsuro.fujita@gmail.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

Richard Guo <guofenglinux@gmail.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 Mon, Aug 29, 2022 at 6:46 PM Richard Guo <guofenglinux@gmail.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 Wed, Aug 31, 2022 at 6:27 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Aug 30, 2022 at 3:50 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> On Mon, Aug 29, 2022 at 6:46 PM Richard Guo <guofenglinux@gmail.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.fujita@gmail.com> wrote:
> On Wed, Aug 31, 2022 at 6:27 PM Richard Guo <guofenglinux@gmail.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 Tue, Sep 13, 2022 at 11:28 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Mon, Sep 12, 2022 at 12:19 PM Etsuro Fujita <etsuro.fujita@gmail.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