Re: POC: postgres_fdw insert batching

On 2/16/21 10:25 AM, Amit Langote wrote:
> On Tue, Feb 16, 2021 at 1:36 AM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>> On 2/5/21 3:52 AM, Amit Langote wrote:
>>> Tsunakwa-san,
>>>
>>> On Mon, Jan 25, 2021 at 1:21 PM tsunakawa(dot)takay(at)fujitsu(dot)com
>>> <tsunakawa(dot)takay(at)fujitsu(dot)com> wrote:
>>>> From: Amit Langote <amitlangote09(at)gmail(dot)com>
>>>>> Yes, it can be simplified by using a local join to prevent the update of the foreign
>>>>> partition from being pushed to the remote server, for which my example in the
>>>>> previous email used a local trigger. Note that the update of the foreign
>>>>> partition to be done locally is a prerequisite for this bug to occur.
>>>>
>>>> Thank you, I was aware that UPDATE calls ExecInsert() but forgot about it partway. Good catch (and my bad miss.)
>>>
>>> It appears I had missed your reply, sorry.
>>>
>>>> + PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState ?
>>>> + (PgFdwModifyState *) resultRelInfo->ri_FdwState :
>>>> + NULL;
>>>>
>>>> This can be written as:
>>>>
>>>> + PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
>>>
>>> Facepalm, yes.
>>>
>>> Patch updated. Thanks for the review.
>>>
>>
>> Thanks for the patch, it seems fine to me.
>
> Thanks for checking.
>
>> I wonder it the commit
>> message needs some tweaks, though. At the moment it says:
>>
>> Prevent FDW insert batching during cross-partition updates
>>
>> but what the patch seems to be doing is simply initializing the info
>> only for CMD_INSERT operations. Which does the trick, but it affects
>> everything, i.e. all updates, no? Not just cross-partition updates.
>
> You're right. Please check the message in the updated patch.
>

Thanks. I'm not sure I understand what "FDW may not be able to handle
both the original update operation and the batched insert operation
being performed at the same time" means. I mean, if we translate the
UPDATE into DELETE+INSERT, then we don't run both the update and insert
at the same time, right? What exactly is the problem with allowing
batching for inserts in cross-partition updates?

On a closer look, it seems the problem actually lies in a small
inconsistency between create_foreign_modify and ExecInitRoutingInfo. The
former only set batch_size for CMD_INSERT while the latter called the
BatchSize() for all operations, expecting >= 1 result. So we may either
relax create_foreign_modify and set batch_size for all DML, or make
ExecInitRoutingInfo stricter (which is what the patches here do).

Is there a reason not to do the first thing, allowing batching of
inserts during cross-partition updates? I tried to do that, but it
dawned on me that we can't mix batched and un-batched operations, e.g.
DELETE + INSERT, because that'd break the order of execution, leading to
bogus results in case the same row is modified repeatedly, etc.

Am I getting this right?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company