Thread: postgres_fdw: Oddity in pushing down inherited UPDATE/DELETE joinsto remote servers

While doing a bit more review of the partitionwise-join-fix patch, I
noticed $SUBJECT.  Here is an example that causes an assertion failure
"TRAP: FailedAssertion("!(foreignrel)", File: "postgres_fdw.c", Line:
2213)":

postgres=# create table t1 (a int, b text);
CREATE TABLE
postgres=# create table t2 (a int, b text);
CREATE TABLE
postgres=# create foreign table ft1 (a int, b text) server loopback
options (table_name 't1');
CREATE FOREIGN TABLE
postgres=# create foreign table ft2 (a int, b text) server loopback
options (table_name 't2');
CREATE FOREIGN TABLE
postgres=# insert into ft1 values (1, 'foo');
INSERT 0 1
postgres=# insert into ft1 values (2, 'bar');
INSERT 0 1
postgres=# insert into ft2 values (1, 'test1');
INSERT 0 1
postgres=# insert into ft2 values (2, 'test2');
INSERT 0 1
postgres=# analyze ft1;
ANALYZE
postgres=# analyze ft2;
ANALYZE
postgres=# create table parent (a int, b text);
CREATE TABLE
postgres=# alter foreign table ft1 inherit parent;
ALTER FOREIGN TABLE
postgres=# update parent set b = ft2.b from ft2 where parent.a = ft2.a;
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.
!>

I think the reason for that is: in that case we try to find the target
foreign-join RelOptInfo using find_join_rel in postgresPlanDirectModify,
but can't find it, because the given root is the *parent* root and
doesn't have join RelOptInfos with it.  To fix this, I'd like to propose
to modify make_modifytable so that in case of an inherited
UPDATE/DELETE, it passes to PlanDirectModify the per-child modified
subroot, not the parent root, for the FDW to get the foreign-join
RelOptInfo from the given root in PlanDirectModify.  I think that that
would be more consistent with the non-inherited UPDATE/DELETE case in
that the FDW can look at any join RelOptInfos in the given root in
PlanDirectModify, which I think would be a good thing because some FDWs
might need to do that for some reason.  For the same reason, I'd also
like to propose to pass to PlanForeignModify the per-child modified
subroot, not the parent root, as for PlanDirectModify.  Attached is a
proposed patch for that.  I'll add this to the open items list for PG11.

Best regards,
Etsuro Fujita

Fujita-san,

On 2018/05/10 21:41, Etsuro Fujita wrote:
> I think the reason for that is: in that case we try to find the target
> foreign-join RelOptInfo using find_join_rel in postgresPlanDirectModify,
> but can't find it, because the given root is the *parent* root and
> doesn't have join RelOptInfos with it.  To fix this, I'd like to propose
> to modify make_modifytable so that in case of an inherited
> UPDATE/DELETE, it passes to PlanDirectModify the per-child modified
> subroot, not the parent root, for the FDW to get the foreign-join
> RelOptInfo from the given root in PlanDirectModify.  I think that that
> would be more consistent with the non-inherited UPDATE/DELETE case in
> that the FDW can look at any join RelOptInfos in the given root in
> PlanDirectModify, which I think would be a good thing because some FDWs
> might need to do that for some reason.  For the same reason, I'd also
> like to propose to pass to PlanForeignModify the per-child modified
> subroot, not the parent root, as for PlanDirectModify.  Attached is a
> proposed patch for that.  I'll add this to the open items list for PG11.

So IIUC, we must pass the per-child PlannerInfo here, because that's what
would have been used for the planning join between the child (ft1 in your
example) and the other table (ft2 in your example).  So that's where the
RelOptInfo's corresponding to planning for the child, including that for
the pushed-down join, would be stored.

Just to clarify, does this problem only arise because there is a pushed
down join involving the child?  That is, does the problem only occur as of
the following commit:

commit 1bc0100d270e5bcc980a0629b8726a32a497e788
Author: Robert Haas <rhaas@postgresql.org>
Date:   Wed Feb 7 15:34:30 2018 -0500

    postgres_fdw: Push down UPDATE/DELETE joins to remote servers.

In other words, do we need to back-patch this up to 9.5 which added
foreign table inheritance?

Anyway, the patch and tests it adds look good.

Thanks,
Amit

On 2018/05/11 16:12, Amit Langote wrote:
> Just to clarify, does this problem only arise because there is a pushed
> down join involving the child?  That is, does the problem only occur as of
> the following commit:
> 
> commit 1bc0100d270e5bcc980a0629b8726a32a497e788
> Author: Robert Haas <rhaas@postgresql.org>
> Date:   Wed Feb 7 15:34:30 2018 -0500
> 
>     postgres_fdw: Push down UPDATE/DELETE joins to remote servers.
> 
> In other words, do we need to back-patch this up to 9.5 which added
> foreign table inheritance?

Oops, it should have been clear by the subject line that the problem
didn't exist before that commit.  Sorry.

Thanks,
Amit

Hi Amit,

(2018/05/11 16:12), Amit Langote wrote:
> On 2018/05/10 21:41, Etsuro Fujita wrote:
>> I think the reason for that is: in that case we try to find the target
>> foreign-join RelOptInfo using find_join_rel in postgresPlanDirectModify,
>> but can't find it, because the given root is the *parent* root and
>> doesn't have join RelOptInfos with it.  To fix this, I'd like to propose
>> to modify make_modifytable so that in case of an inherited
>> UPDATE/DELETE, it passes to PlanDirectModify the per-child modified
>> subroot, not the parent root, for the FDW to get the foreign-join
>> RelOptInfo from the given root in PlanDirectModify.  I think that that
>> would be more consistent with the non-inherited UPDATE/DELETE case in
>> that the FDW can look at any join RelOptInfos in the given root in
>> PlanDirectModify, which I think would be a good thing because some FDWs
>> might need to do that for some reason.  For the same reason, I'd also
>> like to propose to pass to PlanForeignModify the per-child modified
>> subroot, not the parent root, as for PlanDirectModify.  Attached is a
>> proposed patch for that.  I'll add this to the open items list for PG11.
> 
> So IIUC, we must pass the per-child PlannerInfo here, because that's what
> would have been used for the planning join between the child (ft1 in your
> example) and the other table (ft2 in your example).  So that's where the
> RelOptInfo's corresponding to planning for the child, including that for
> the pushed-down join, would be stored.

Yeah, I think so too.

> Anyway, the patch and tests it adds look good.

Thanks for the review!

I added an assertion test to make_modifytable to match that in
create_modifytable_path.  Attached is an updated version of the patch.

Best regards,
Etsuro Fujita

(2018/05/11 16:19), Amit Langote wrote:
> On 2018/05/11 16:12, Amit Langote wrote:
>> Just to clarify, does this problem only arise because there is a pushed
>> down join involving the child?  That is, does the problem only occur as of
>> the following commit:
>>
>> commit 1bc0100d270e5bcc980a0629b8726a32a497e788
>> Author: Robert Haas<rhaas@postgresql.org>
>> Date:   Wed Feb 7 15:34:30 2018 -0500
>>
>>      postgres_fdw: Push down UPDATE/DELETE joins to remote servers.
>>
>> In other words, do we need to back-patch this up to 9.5 which added
>> foreign table inheritance?
> 
> Oops, it should have been clear by the subject line that the problem
> didn't exist before that commit.  Sorry.

No.  In theory, I think we could consider this as an older bug added in
9.5, because in case of inherited UPDATE/DELETE, the PlannerInfo passed
to PlanForeignModify doesn't match the one the FDW saw at Path creation
time, as you mentioned in a previous email, while in case of
non-inherited UPDATE/DELETE, the PlannerInfo passed to that function
matches the one the FDW saw at that time.  I think that's my fault :(.
But considering there seems to be no field reports on that, I don't
think we need back-patching up to 9.5.

Best regards,
Etsuro Fujita

On 2018/05/11 21:48, Etsuro Fujita wrote:
> (2018/05/11 16:19), Amit Langote wrote:
>> On 2018/05/11 16:12, Amit Langote wrote:
>>> Just to clarify, does this problem only arise because there is a pushed
>>> down join involving the child?  That is, does the problem only occur as of
>>> the following commit:
>>>
>>> commit 1bc0100d270e5bcc980a0629b8726a32a497e788
>>> Author: Robert Haas<rhaas@postgresql.org>
>>> Date:   Wed Feb 7 15:34:30 2018 -0500
>>>
>>>      postgres_fdw: Push down UPDATE/DELETE joins to remote servers.
>>>
>>> In other words, do we need to back-patch this up to 9.5 which added
>>> foreign table inheritance?
>>
>> Oops, it should have been clear by the subject line that the problem
>> didn't exist before that commit.  Sorry.
> 
> No.  In theory, I think we could consider this as an older bug added in
> 9.5, because in case of inherited UPDATE/DELETE, the PlannerInfo passed
> to PlanForeignModify doesn't match the one the FDW saw at Path creation
> time, as you mentioned in a previous email, while in case of
> non-inherited UPDATE/DELETE, the PlannerInfo passed to that function
> matches the one the FDW saw at that time.  I think that's my fault :(.

Ah, I see.  Thanks for clarifying.

> But considering there seems to be no field reports on that, I don't
> think we need back-patching up to 9.5.

Yeah, that might be fine, although it perhaps wouldn't hurt to have the
code match in all branches.

Thanks,
Amit

(2018/05/14 9:45), Amit Langote wrote:
> On 2018/05/11 21:48, Etsuro Fujita wrote:
>> (2018/05/11 16:19), Amit Langote wrote:
>>> On 2018/05/11 16:12, Amit Langote wrote:
>>>> Just to clarify, does this problem only arise because there is a pushed
>>>> down join involving the child?  That is, does the problem only occur as of
>>>> the following commit:
>>>>
>>>> commit 1bc0100d270e5bcc980a0629b8726a32a497e788
>>>> Author: Robert Haas<rhaas@postgresql.org>
>>>> Date:   Wed Feb 7 15:34:30 2018 -0500
>>>>
>>>>       postgres_fdw: Push down UPDATE/DELETE joins to remote servers.
>>>>
>>>> In other words, do we need to back-patch this up to 9.5 which added
>>>> foreign table inheritance?
>>>
>>> Oops, it should have been clear by the subject line that the problem
>>> didn't exist before that commit.  Sorry.
>>
>> No.  In theory, I think we could consider this as an older bug added in
>> 9.5, because in case of inherited UPDATE/DELETE, the PlannerInfo passed
>> to PlanForeignModify doesn't match the one the FDW saw at Path creation
>> time, as you mentioned in a previous email, while in case of
>> non-inherited UPDATE/DELETE, the PlannerInfo passed to that function
>> matches the one the FDW saw at that time.  I think that's my fault :(.
> 
> Ah, I see.  Thanks for clarifying.
> 
>> But considering there seems to be no field reports on that, I don't
>> think we need back-patching up to 9.5.
> 
> Yeah, that might be fine, although it perhaps wouldn't hurt to have the
> code match in all branches.

I don't object to back-patching.  Should I remove this from the open
items list for PG11?

Best regards,
Etsuro Fujita

Fujita-san,

On 2018/05/16 18:35, Etsuro Fujita wrote:
> (2018/05/14 9:45), Amit Langote wrote:
>> On 2018/05/11 21:48, Etsuro Fujita wrote:
>>> (2018/05/11 16:19), Amit Langote wrote:
>>>> On 2018/05/11 16:12, Amit Langote wrote:
>>>>> Just to clarify, does this problem only arise because there is a pushed
>>>>> down join involving the child?  That is, does the problem only occur as of
>>>>> the following commit:
>>>>>
>>>>> commit 1bc0100d270e5bcc980a0629b8726a32a497e788
>>>>> Author: Robert Haas<rhaas@postgresql.org>
>>>>> Date:   Wed Feb 7 15:34:30 2018 -0500
>>>>>
>>>>>       postgres_fdw: Push down UPDATE/DELETE joins to remote servers.
>>>>>
>>>>> In other words, do we need to back-patch this up to 9.5 which added
>>>>> foreign table inheritance?
>>>>
>>>> Oops, it should have been clear by the subject line that the problem
>>>> didn't exist before that commit.  Sorry.
>>>
>>> No.  In theory, I think we could consider this as an older bug added in
>>> 9.5, because in case of inherited UPDATE/DELETE, the PlannerInfo passed
>>> to PlanForeignModify doesn't match the one the FDW saw at Path creation
>>> time, as you mentioned in a previous email, while in case of
>>> non-inherited UPDATE/DELETE, the PlannerInfo passed to that function
>>> matches the one the FDW saw at that time.  I think that's my fault :(.
>>
>> Ah, I see.  Thanks for clarifying.
>>
>>> But considering there seems to be no field reports on that, I don't
>>> think we need back-patching up to 9.5.
>>
>> Yeah, that might be fine, although it perhaps wouldn't hurt to have the
>> code match in all branches.
> 
> I don't object to back-patching.  Should I remove this from the open
> items list for PG11?

Perhaps, keep on the page but in Older Bugs section?

Thanks,
Amit

(2018/05/16 18:40), Amit Langote wrote:
> On 2018/05/16 18:35, Etsuro Fujita wrote:
>> I don't object to back-patching.  Should I remove this from the open
>> items list for PG11?
> 
> Perhaps, keep on the page but in Older Bugs section?

OK, I'll move to Older Bugs on that page and add to the next commitfest.

Best regards,
Etsuro Fujita

On Fri, May 11, 2018 at 8:46 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> I added an assertion test to make_modifytable to match that in
> create_modifytable_path.  Attached is an updated version of the patch.

Committed.  Was it just good luck that this ever worked at all?  I mean:

-        if (rti < root->simple_rel_array_size &&
-            root->simple_rel_array[rti] != NULL)
+        if (rti < subroot->simple_rel_array_size &&
+            subroot->simple_rel_array[rti] != NULL)
         {
-            RelOptInfo *resultRel = root->simple_rel_array[rti];
+            RelOptInfo *resultRel = subroot->simple_rel_array[rti];

             fdwroutine = resultRel->fdwroutine;
         }
         else
         {
-            RangeTblEntry *rte = planner_rt_fetch(rti, root);
+            RangeTblEntry *rte = planner_rt_fetch(rti, subroot);

...an RTI is only meaningful relative to a particular PlannerInfo's
range table.  It can't be right to just take an RTI for one
PlannerInfo and look it up in some other PlannerInfo's range table.

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

(2018/05/17 1:16), Robert Haas wrote:
> On Fri, May 11, 2018 at 8:46 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp>  wrote:
>> I added an assertion test to make_modifytable to match that in
>> create_modifytable_path.  Attached is an updated version of the patch.
>
> Committed.

Thanks you!

> Was it just good luck that this ever worked at all?  I mean:
>
> -        if (rti<  root->simple_rel_array_size&&
> -            root->simple_rel_array[rti] != NULL)
> +        if (rti<  subroot->simple_rel_array_size&&
> +            subroot->simple_rel_array[rti] != NULL)
>           {
> -            RelOptInfo *resultRel = root->simple_rel_array[rti];
> +            RelOptInfo *resultRel = subroot->simple_rel_array[rti];
>
>               fdwroutine = resultRel->fdwroutine;
>           }
>           else
>           {
> -            RangeTblEntry *rte = planner_rt_fetch(rti, root);
> +            RangeTblEntry *rte = planner_rt_fetch(rti, subroot);
>
> ...an RTI is only meaningful relative to a particular PlannerInfo's
> range table.  It can't be right to just take an RTI for one
> PlannerInfo and look it up in some other PlannerInfo's range table.

I think that can be right; because inheritance_planner generates the 
simple_rel_array and simple_rte_array for the parent PlannerInfo so that 
it allows us to do that.  This is the logic that that function creates 
the simple_rel_array for the parent PlannerInfo:

         /*
          * We need to collect all the RelOptInfos from all child plans into
          * the main PlannerInfo, since setrefs.c will need them.  We 
use the
          * last child's simple_rel_array (previous ones are too short), 
so we
          * have to propagate forward the RelOptInfos that were already 
built
          * in previous children.
          */
         Assert(subroot->simple_rel_array_size >= save_rel_array_size);
         for (rti = 1; rti < save_rel_array_size; rti++)
         {
             RelOptInfo *brel = save_rel_array[rti];

             if (brel)
                 subroot->simple_rel_array[rti] = brel;
         }

Best regards,
Etsuro Fujita

On 2018/05/17 12:30, Etsuro Fujita wrote:
> (2018/05/17 1:16), Robert Haas wrote:
>> Was it just good luck that this ever worked at all?  I mean:
>>
>> -        if (rti<  root->simple_rel_array_size&&
>> -            root->simple_rel_array[rti] != NULL)
>> +        if (rti<  subroot->simple_rel_array_size&&
>> +            subroot->simple_rel_array[rti] != NULL)
>>           {
>> -            RelOptInfo *resultRel = root->simple_rel_array[rti];
>> +            RelOptInfo *resultRel = subroot->simple_rel_array[rti];
>>
>>               fdwroutine = resultRel->fdwroutine;
>>           }
>>           else
>>           {
>> -            RangeTblEntry *rte = planner_rt_fetch(rti, root);
>> +            RangeTblEntry *rte = planner_rt_fetch(rti, subroot);
>>
>> ...an RTI is only meaningful relative to a particular PlannerInfo's
>> range table.  It can't be right to just take an RTI for one
>> PlannerInfo and look it up in some other PlannerInfo's range table.
> 
> I think that can be right; because inheritance_planner generates the
> simple_rel_array and simple_rte_array for the parent PlannerInfo so that
> it allows us to do that.  This is the logic that that function creates the
> simple_rel_array for the parent PlannerInfo:
> 
>         /*
>          * We need to collect all the RelOptInfos from all child plans into
>          * the main PlannerInfo, since setrefs.c will need them.  We use the
>          * last child's simple_rel_array (previous ones are too short), so we
>          * have to propagate forward the RelOptInfos that were already built
>          * in previous children.
>          */
>         Assert(subroot->simple_rel_array_size >= save_rel_array_size);
>         for (rti = 1; rti < save_rel_array_size; rti++)
>         {
>             RelOptInfo *brel = save_rel_array[rti];
> 
>             if (brel)
>                 subroot->simple_rel_array[rti] = brel;
>         }

Looking at this for a bit, I wondered if this crash wouldn't have occurred
if the "propagation" had also considered join relations in addition to
simple relations.  For example, if I changed inheritance_planner like the
attached (not proposing that we consider committing it), reported crash
doesn't occur.  The fact that it's not currently that way means that
somebody thought that there is no point in keeping all of those joinrels
around until plan creation time.  If that is so, is it a bit worrying that
a FDW function invoked from createplan.c may try to look for one?

Thanks,
Amit

On 2018/05/17 14:19, Amit Langote wrote:
> Looking at this for a bit, I wondered if this crash wouldn't have occurred
> if the "propagation" had also considered join relations in addition to
> simple relations.  For example, if I changed inheritance_planner like the
> attached (not proposing that we consider committing it), reported crash
> doesn't occur.  The fact that it's not currently that way means that
> somebody thought that there is no point in keeping all of those joinrels
> around until plan creation time.  If that is so, is it a bit worrying that
> a FDW function invoked from createplan.c may try to look for one?

Oops, I forgot to attach the patch that I had used in the experiment.

Thanks,
Amit

(2018/05/17 14:19), Amit Langote wrote:
> Looking at this for a bit, I wondered if this crash wouldn't have occurred
> if the "propagation" had also considered join relations in addition to
> simple relations.  For example, if I changed inheritance_planner like the
> attached (not proposing that we consider committing it), reported crash
> doesn't occur.  The fact that it's not currently that way means that
> somebody thought that there is no point in keeping all of those joinrels
> around until plan creation time.

One reason for that would be that we use the per-child PlannerInfo, not 
the parent one, at plan creation time.  Here is the code in 
create_modifytable_plan:

         /*
          * In an inherited UPDATE/DELETE, reference the per-child modified
          * subroot while creating Plans from Paths for the child rel. 
This is
          * a kluge, but otherwise it's too hard to ensure that Plan 
creation
          * functions (particularly in FDWs) don't depend on the contents of
          * "root" matching what they saw at Path creation time.  The main
          * downside is that creation functions for Plans that might appear
          * below a ModifyTable cannot expect to modify the contents of 
"root"
          * and have it "stick" for subsequent processing such as setrefs.c.
          * That's not great, but it seems better than the alternative.
          */
         subplan = create_plan_recurse(subroot, subpath, CP_EXACT_TLIST);

So, we don't need to accumulate the joinrel lists for child relations 
into a single list and store that list into the parent PlannerInfo in 
inheritance_planner, as in the patch you proposed.  I think the change 
by the commit is based on the same idea as that.

Best regards,
Etsuro Fujita

Fujita-san,

On 2018/05/17 21:51, Etsuro Fujita wrote:
> (2018/05/17 14:19), Amit Langote wrote:
>> Looking at this for a bit, I wondered if this crash wouldn't have occurred
>> if the "propagation" had also considered join relations in addition to
>> simple relations.  For example, if I changed inheritance_planner like the
>> attached (not proposing that we consider committing it), reported crash
>> doesn't occur.  The fact that it's not currently that way means that
>> somebody thought that there is no point in keeping all of those joinrels
>> around until plan creation time.
> 
> One reason for that would be that we use the per-child PlannerInfo, not
> the parent one, at plan creation time.  Here is the code in
> create_modifytable_plan:
> 
>         /*
>          * In an inherited UPDATE/DELETE, reference the per-child modified
>          * subroot while creating Plans from Paths for the child rel. This is
>          * a kluge, but otherwise it's too hard to ensure that Plan creation
>          * functions (particularly in FDWs) don't depend on the contents of
>          * "root" matching what they saw at Path creation time.  The main
>          * downside is that creation functions for Plans that might appear
>          * below a ModifyTable cannot expect to modify the contents of "root"
>          * and have it "stick" for subsequent processing such as setrefs.c.
>          * That's not great, but it seems better than the alternative.
>          */
>         subplan = create_plan_recurse(subroot, subpath, CP_EXACT_TLIST);
> 
> So, we don't need to accumulate the joinrel lists for child relations into
> a single list and store that list into the parent PlannerInfo in
> inheritance_planner, as in the patch you proposed.  I think the change by
> the commit is based on the same idea as that.

OK, thanks for explaining.  I am not questioning what's already committed
as a fix for this issue, nor am I proposing the patch that I posted
earlier to be considered for committing.  I was just idly wondering why
PlanDirectModifyTable looks for joinrel RelOptInfos in the plan creation
phase if they're not guaranteed to exist, especially if no other plan
creation functions do.  For instance, postgres_fdw's
postgresPlanDirectModify appears to be the only function that's invoked in
the plan creation phase that's doing a find_join_rel().  Maybe, the rule
that joinrels shouldn't be accessed this late doesn't exist and hence we
shouldn't be worried.

Thanks,
Amit