RE: Allow batched insert during cross-partition updates

From: "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Zhihong Yu <zyu(at)yugabyte(dot)com>, "gkokolatos(at)pm(dot)me" <gkokolatos(at)pm(dot)me>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, "tsunakawa(dot)takay(at)fujitsu(dot)com" <tsunakawa(dot)takay(at)fujitsu(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Subject: RE: Allow batched insert during cross-partition updates
Date: 2021-05-07 09:39:37
Message-ID: OS0PR01MB5716DFBD5A10BD8E49EA3C5B94579@OS0PR01MB5716.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> > Thanks! It looks good!
>
> Thanks for checking. I'll mark this as RfC.

Hi,

The patch cannot be applied to the latest head branch, it will be nice if you can rebase it.
And when looking into the patch, I have some comments on it.

1)
IIRC, After the commit c5b7ba4, the initialization of mt_partition_tuple_routing was postponed out of ExecInitModifyTable.
So, the following if-test use "proute" which is initialized at the beginning of the ExecModifyTable() could be out of date.
And the regression test of postgres_fdw failed with the patch after the commit c5b7ba4.

+ * If the query's main target relation is a partitioned table, any inserts
+ * would have been performed using tuple routing, so use the appropriate
+ * set of target relations. Note that this also covers any inserts
+ * performed during cross-partition UPDATEs that would have occurred
+ * through tuple routing.
*/
if (proute)
...

It seems we should get the mt_partition_tuple_routing just before the if-test.

2)
+ foreach(lc, estate->es_opened_result_relations)
+ {
+ resultRelInfo = lfirst(lc);
+ if (resultRelInfo &&

I am not sure do we need to check if resultRelInfo == NULL because the the existing code did not check it.
And if we need to check it, it might be better use "if (resultRelInfo == NULL &&..."

3)
+ if (fmstate && fmstate->aux_fmstate != NULL)
+ fmstate = fmstate->aux_fmstate;

It might be better to write like " if (fmstate != NULL && fmstate->aux_fmstate != NULL)".

Best regards,
houzj

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2021-05-07 09:42:05 Re: few ideas for pgbench
Previous Message Fabien COELHO 2021-05-07 09:28:49 Re: few ideas for pgbench