Re: Problem with transition tables on partitioned tables with foreign-table partitions

Amit-san,

On Thu, Jul 10, 2025 at 11:54 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Jul 10, 2025 at 22:20 Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> On Wed, Jul 9, 2025 at 5:07 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
>> > Here is an updated version of the patch, in which I added 1) an Assert
>> > to ExecAR* functions to ensure that the passed-in ResultRelInfo
>> > pointer is not NULL, 2) added/tweaked comments a bit more, and 3)
>> > added regression tests.
>>
>> Thanks for the new patch. LGTM.

Cool!

>> While reading it, I noticed that the functions performing table_open()
>> are repeatedly called in this condition, which runs for every
>> qualifying foreign child relations:
>>
>> if (fdwroutine != NULL &&
>> fdwroutine->PlanDirectModify != NULL &&
>> fdwroutine->BeginDirectModify != NULL &&
>> fdwroutine->IterateDirectModify != NULL &&
>> fdwroutine->EndDirectModify != NULL &&
>> withCheckOptionLists == NIL &&
>> !has_row_triggers(root, rti, operation) &&
>> !has_stored_generated_columns(root, rti))
>>
>> That seems a bit expensive. It might be worth using *_valid flags to
>> avoid redundant table_open() calls like you're doing for transition
>> table checking. Maybe something to consider in a separate patch.

> Ah, scratch that because I missed that transition table checking is done for the “named” relation and these are checking it for child relations.

Ok, thanks for taking the time for this patch!

After re-reading the patch I noticed two minor things:

* The existing code in ExecAR* functions already dereferences the
passed-in ResultRelInfo pointer without checking that it is not NULL.
The Assert I added to those functions would be an overkill, so I
removed it. Sorry for the back and forth.

* I added a trigger function trigger_nothing() in the regression
tests, but I noticed an existing trigger function above the tests. To
make the tests a bit small, I replaced trigger_nothing() with the
existing trigger function and removed trigger_nothing().

Attached is a new version of the patch.

Best regards,
Etsuro Fujita