Horton

Thread

Commits

  1. Fix thinko in ExecCleanupTupleRouting().

  2. Redesign initialization of partition routing structures

  1. Issue in ExecCleanupTupleRouting()

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2019-04-11T13:05:19Z

    Hi,

(added Alvaro, Amit, and David)

While working on an update-tuple-routing bug in postgres_fdw [1], I
noticed this change to ExecCleanupTupleRouting() made by commit
3f2393edefa5ef2b6970a5a2fa2c7e9c55cc10cf:

+       /*
+        * Check if this result rel is one belonging to the node's subplans,
+        * if so, let ExecEndPlan() clean it up.
+        */
+       if (htab)
+       {
+           Oid         partoid;
+           bool        found;
+
+           partoid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
+
+           (void) hash_search(htab, &partoid, HASH_FIND, &found);
+           if (found)
+               continue;
+       }

        /* Allow any FDWs to shut down if they've been exercised */
-       if (resultRelInfo->ri_PartitionReadyForRouting &&
-           resultRelInfo->ri_FdwRoutine != NULL &&
+       if (resultRelInfo->ri_FdwRoutine != NULL &&
            resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)

resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state,
                                                           resultRelInfo);

This skips subplan resultrels before calling EndForeignInsert() if they
are foreign tables, which I think causes an issue: the FDWs would fail
to release resources for their foreign insert operations, because
ExecEndPlan() and ExecEndModifyTable() don't do anything to allow them
to do that.  So I think we should skip subplan resultrels after
EndForeignInsert().  Attached is a small patch for that.

Best regards,
Etsuro Fujita

[1]
https://www.postgresql.org/message-id/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440%40lab.ntt.co.jp

  2. Re: Issue in ExecCleanupTupleRouting()

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-11T13:28:04Z

    On Fri, 12 Apr 2019 at 01:06, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
> +       /*
> +        * Check if this result rel is one belonging to the node's subplans,
> +        * if so, let ExecEndPlan() clean it up.
> +        */
> +       if (htab)
> +       {
> +           Oid         partoid;
> +           bool        found;
> +
> +           partoid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
> +
> +           (void) hash_search(htab, &partoid, HASH_FIND, &found);
> +           if (found)
> +               continue;
> +       }
>
>         /* Allow any FDWs to shut down if they've been exercised */
> -       if (resultRelInfo->ri_PartitionReadyForRouting &&
> -           resultRelInfo->ri_FdwRoutine != NULL &&
> +       if (resultRelInfo->ri_FdwRoutine != NULL &&
>             resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
>
> resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state,
>                                                            resultRelInfo);
>
> This skips subplan resultrels before calling EndForeignInsert() if they
> are foreign tables, which I think causes an issue: the FDWs would fail
> to release resources for their foreign insert operations, because
> ExecEndPlan() and ExecEndModifyTable() don't do anything to allow them
> to do that.  So I think we should skip subplan resultrels after
> EndForeignInsert().  Attached is a small patch for that.

Oops. I had for some reason been under the impression that it was
nodeModifyTable.c, or whatever the calling code happened to be that
handles these ones, but this is not the case as we call
ExecInitRoutingInfo() from ExecFindPartition() which makes the call to
BeginForeignInsert. If that part is handled by the tuple routing code,
then the subsequent cleanup should be too, in which case your patch
looks fine.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  3. Re: Issue in ExecCleanupTupleRouting()

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2019-04-12T01:48:06Z

    On 2019/04/11 22:28, David Rowley wrote:
> On Fri, 12 Apr 2019 at 01:06, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
>> +       /*
>> +        * Check if this result rel is one belonging to the node's subplans,
>> +        * if so, let ExecEndPlan() clean it up.
>> +        */
>> +       if (htab)
>> +       {
>> +           Oid         partoid;
>> +           bool        found;
>> +
>> +           partoid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
>> +
>> +           (void) hash_search(htab, &partoid, HASH_FIND, &found);
>> +           if (found)
>> +               continue;
>> +       }
>>
>>         /* Allow any FDWs to shut down if they've been exercised */
>> -       if (resultRelInfo->ri_PartitionReadyForRouting &&
>> -           resultRelInfo->ri_FdwRoutine != NULL &&
>> +       if (resultRelInfo->ri_FdwRoutine != NULL &&
>>             resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
>>
>> resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state,
>>                                                            resultRelInfo);
>>
>> This skips subplan resultrels before calling EndForeignInsert() if they
>> are foreign tables, which I think causes an issue: the FDWs would fail
>> to release resources for their foreign insert operations, because
>> ExecEndPlan() and ExecEndModifyTable() don't do anything to allow them
>> to do that.  So I think we should skip subplan resultrels after
>> EndForeignInsert().  Attached is a small patch for that.
> 
> Oops. I had for some reason been under the impression that it was
> nodeModifyTable.c, or whatever the calling code happened to be that
> handles these ones, but this is not the case as we call
> ExecInitRoutingInfo() from ExecFindPartition() which makes the call to
> BeginForeignInsert. If that part is handled by the tuple routing code,
> then the subsequent cleanup should be too, in which case your patch
> looks fine.

That sounds right.

Thanks,
Amit

  4. Re: Issue in ExecCleanupTupleRouting()

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-15T04:32:53Z

    On Fri, 12 Apr 2019 at 01:06, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
> While working on an update-tuple-routing bug in postgres_fdw [1], I
> noticed this change to ExecCleanupTupleRouting() made by commit
> 3f2393edefa5ef2b6970a5a2fa2c7e9c55cc10cf:

Added to open items list.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  5. Re: Issue in ExecCleanupTupleRouting()

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2019-04-15T10:12:23Z

    (2019/04/12 10:48), Amit Langote wrote:
> On 2019/04/11 22:28, David Rowley wrote:
>> On Fri, 12 Apr 2019 at 01:06, Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp>  wrote:
>>> +       /*
>>> +        * Check if this result rel is one belonging to the node's subplans,
>>> +        * if so, let ExecEndPlan() clean it up.
>>> +        */
>>> +       if (htab)
>>> +       {
>>> +           Oid         partoid;
>>> +           bool        found;
>>> +
>>> +           partoid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
>>> +
>>> +           (void) hash_search(htab,&partoid, HASH_FIND,&found);
>>> +           if (found)
>>> +               continue;
>>> +       }
>>>
>>>          /* Allow any FDWs to shut down if they've been exercised */
>>> -       if (resultRelInfo->ri_PartitionReadyForRouting&&
>>> -           resultRelInfo->ri_FdwRoutine != NULL&&
>>> +       if (resultRelInfo->ri_FdwRoutine != NULL&&
>>>              resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
>>>
>>> resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state,
>>>                                                             resultRelInfo);
>>>
>>> This skips subplan resultrels before calling EndForeignInsert() if they
>>> are foreign tables, which I think causes an issue: the FDWs would fail
>>> to release resources for their foreign insert operations, because
>>> ExecEndPlan() and ExecEndModifyTable() don't do anything to allow them
>>> to do that.  So I think we should skip subplan resultrels after
>>> EndForeignInsert().  Attached is a small patch for that.
>>
>> Oops. I had for some reason been under the impression that it was
>> nodeModifyTable.c, or whatever the calling code happened to be that
>> handles these ones, but this is not the case as we call
>> ExecInitRoutingInfo() from ExecFindPartition() which makes the call to
>> BeginForeignInsert. If that part is handled by the tuple routing code,
>> then the subsequent cleanup should be too, in which case your patch
>> looks fine.
>
> That sounds right.

Pushed.  Thanks for reviewing, David and Amit!

Best regards,
Etsuro Fujita

  6. Re: Issue in ExecCleanupTupleRouting()

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2019-04-15T10:16:09Z

    (2019/04/15 13:32), David Rowley wrote:
> On Fri, 12 Apr 2019 at 01:06, Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp>  wrote:
>> While working on an update-tuple-routing bug in postgres_fdw [1], I
>> noticed this change to ExecCleanupTupleRouting() made by commit
>> 3f2393edefa5ef2b6970a5a2fa2c7e9c55cc10cf:
>
> Added to open items list.

Thanks!  I moved this item to resolved ones.

Best regards,
Etsuro Fujita