Re: Allow batched insert during cross-partition updates
On Tue, Dec 20, 2022 at 7:18 PM Etsuro Fujita wrote: > On Wed, Dec 14, 2022 at 10:29 PM Amit Langote wrote: > > On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita > > wrote: > > > One thing I noticed is this bit: > > > > > > -- Clean up > > > -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0, > > > batch_table_p1 CASCADE; > > > +DROP TABLE batch_table, batch_table_p0, batch_table_p1, > > > batch_cp_upd_test, cmdlog CASCADE; > > > > > > This would be nitpicking, but this as-proposed will not remove remote > > > tables created for foreign-table partitions of the partitioned table > > > ‘batch_cp_upd_test’. So I modified this a bit further to remove them > > > as well. Also, I split this into two, for readability. Another thing > > > is a typo in a test-case comment: s/a single INSERTs/a single INSERT/. > > > I fixed it as well. Other than that, the patch looks good to me. > > > Attached is an updated patch. If there are no objections, I will > > > commit the patch. > > > > LGTM. > > Cool! Pushed. Thank you, Fujita-san. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
Hi Amit-san, On Wed, Dec 14, 2022 at 10:29 PM Amit Langote wrote: > On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita wrote: > > One thing I noticed is this bit: > > > > -- Clean up > > -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0, > > batch_table_p1 CASCADE; > > +DROP TABLE batch_table, batch_table_p0, batch_table_p1, > > batch_cp_upd_test, cmdlog CASCADE; > > > > This would be nitpicking, but this as-proposed will not remove remote > > tables created for foreign-table partitions of the partitioned table > > ‘batch_cp_upd_test’. So I modified this a bit further to remove them > > as well. Also, I split this into two, for readability. Another thing > > is a typo in a test-case comment: s/a single INSERTs/a single INSERT/. > > I fixed it as well. Other than that, the patch looks good to me. > > Attached is an updated patch. If there are no objections, I will > > commit the patch. > > LGTM. Cool! Pushed. Best regards, Etsuro Fujita
Re: Allow batched insert during cross-partition updates
On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita wrote: > On Thu, Dec 8, 2022 at 8:01 PM Etsuro Fujita wrote: > > On Thu, Dec 8, 2022 at 5:00 PM Amit Langote wrote: > > > Updated patch attached. > > > > I will review the patch a bit more, but I think > > it would be committable. > > One thing I noticed is this bit: > > -- Clean up > -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0, > batch_table_p1 CASCADE; > +DROP TABLE batch_table, batch_table_p0, batch_table_p1, > batch_cp_upd_test, cmdlog CASCADE; > > This would be nitpicking, but this as-proposed will not remove remote > tables created for foreign-table partitions of the partitioned table > ‘batch_cp_upd_test’. So I modified this a bit further to remove them > as well. Also, I split this into two, for readability. Another thing > is a typo in a test-case comment: s/a single INSERTs/a single INSERT/. > I fixed it as well. Other than that, the patch looks good to me. > Attached is an updated patch. If there are no objections, I will > commit the patch. Thanks for the changes. LGTM. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
On Thu, Dec 8, 2022 at 8:01 PM Etsuro Fujita wrote: > On Thu, Dec 8, 2022 at 5:00 PM Amit Langote wrote: > > Updated patch attached. > > I will review the patch a bit more, but I think > it would be committable. One thing I noticed is this bit: -- Clean up -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0, batch_table_p1 CASCADE; +DROP TABLE batch_table, batch_table_p0, batch_table_p1, batch_cp_upd_test, cmdlog CASCADE; This would be nitpicking, but this as-proposed will not remove remote tables created for foreign-table partitions of the partitioned table ‘batch_cp_upd_test’. So I modified this a bit further to remove them as well. Also, I split this into two, for readability. Another thing is a typo in a test-case comment: s/a single INSERTs/a single INSERT/. I fixed it as well. Other than that, the patch looks good to me. Attached is an updated patch. If there are no objections, I will commit the patch. Best regards, Etsuro Fujita v11-0001-Allow-batching-of-inserts-during-cross-partition-efujita.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
Amit-san, On Thu, Dec 8, 2022 at 5:00 PM Amit Langote wrote: > On Tue, Dec 6, 2022 at 6:47 PM Etsuro Fujita wrote: > > * In postgresGetForeignModifyBatchSize(): > > > > /* > > -* Should never get called when the insert is being performed as part > > of a > > -* row movement operation. > > +* Use the auxiliary state if any; see postgresBeginForeignInsert() for > > +* details on what it represents. > > */ > > - Assert(fmstate == NULL || fmstate->aux_fmstate == NULL); > > + if (fmstate != NULL && fmstate->aux_fmstate != NULL) > > + fmstate = fmstate->aux_fmstate; > > > > I might be missing something, but I think we should leave the Assert > > as-is, because we still disallow to move rows to another foreign-table > > partition that is also an UPDATE subplan result relation, which means > > that any fmstate should have fmstate->aux_fmstate=NULL. > > Hmm, yes. I forgot that 86dc90056df effectively disabled *all* > attempts of inserting into foreign partitions that are also UPDATE > target relations, so you are correct that fmstate->aux_fmstate would > never be set when entering this function. > > That means this functionality is only useful for foreign partitions > that are not also being updated by the original UPDATE. Yeah, I think so too. > I've reinstated the Assert, removed the if block as it's useless, and > updated the comment a bit to clarify the restriction a bit. Looks good to me. > > * Also in that function: > > > > - if (fmstate) > > + if (fmstate != NULL) > > > > This is correct, but I would vote for leaving that as-is, to make > > back-patching easy. > > Removed this hunk. Thanks! > Updated patch attached. Thanks for the patch! I will review the patch a bit more, but I think it would be committable. Best regards, Etsuro Fujita
Re: Allow batched insert during cross-partition updates
Hi Fujita-san, On Tue, Dec 6, 2022 at 6:47 PM Etsuro Fujita wrote: > On Tue, Mar 22, 2022 at 10:17 AM Amit Langote wrote: > > Rebased to fix a minor conflict with some recently committed > > nodeModifyTable.c changes. > > Apologies for not having reviewed the patch. Here are some review comments: No problem and thanks for taking a look. > * The patch conflicts with commit ffbb7e65a, so please update the > patch. (That commit would cause an API break, so I am planning to > apply a fix to HEAD as well [1].) That commit fixed the handling of > pending inserts, which I think would eliminate the need to do this: > > * ExecModifyTable(), when inserting any remaining batched tuples, > must look at the correct set of ResultRelInfos that would've been > used by such inserts, because failing to do so would result in those > tuples not actually getting inserted. To fix, ExecModifyTable() is > now made to get the ResultRelInfos from the PartitionTupleRouting > data structure which contains the ResultRelInfo that would be used by > those internal inserts. To allow nodeModifyTable.c look inside > PartitionTupleRouting, its definition, which was previously local to > execPartition.c, is exposed via execPartition.h. Ah, I see. Removed those hunks. > * In postgresGetForeignModifyBatchSize(): > > /* > -* Should never get called when the insert is being performed as part of a > -* row movement operation. > +* Use the auxiliary state if any; see postgresBeginForeignInsert() for > +* details on what it represents. > */ > - Assert(fmstate == NULL || fmstate->aux_fmstate == NULL); > + if (fmstate != NULL && fmstate->aux_fmstate != NULL) > + fmstate = fmstate->aux_fmstate; > > I might be missing something, but I think we should leave the Assert > as-is, because we still disallow to move rows to another foreign-table > partition that is also an UPDATE subplan result relation, which means > that any fmstate should have fmstate->aux_fmstate=NULL. Hmm, yes. I forgot that 86dc90056df effectively disabled *all* attempts of inserting into foreign partitions that are also UPDATE target relations, so you are correct that fmstate->aux_fmstate would never be set when entering this function. That means this functionality is only useful for foreign partitions that are not also being updated by the original UPDATE. I've reinstated the Assert, removed the if block as it's useless, and updated the comment a bit to clarify the restriction a bit. > * Also in that function: > > - if (fmstate) > + if (fmstate != NULL) > > This is correct, but I would vote for leaving that as-is, to make > back-patching easy. Removed this hunk. > That is all I have for now. I will mark this as Waiting on Author. Updated patch attached. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v11-0001-Allow-batching-of-inserts-during-cross-partition.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
Amit-san, On Tue, Mar 22, 2022 at 10:17 AM Amit Langote wrote: > Rebased to fix a minor conflict with some recently committed > nodeModifyTable.c changes. Apologies for not having reviewed the patch. Here are some review comments: * The patch conflicts with commit ffbb7e65a, so please update the patch. (That commit would cause an API break, so I am planning to apply a fix to HEAD as well [1].) That commit fixed the handling of pending inserts, which I think would eliminate the need to do this: * ExecModifyTable(), when inserting any remaining batched tuples, must look at the correct set of ResultRelInfos that would've been used by such inserts, because failing to do so would result in those tuples not actually getting inserted. To fix, ExecModifyTable() is now made to get the ResultRelInfos from the PartitionTupleRouting data structure which contains the ResultRelInfo that would be used by those internal inserts. To allow nodeModifyTable.c look inside PartitionTupleRouting, its definition, which was previously local to execPartition.c, is exposed via execPartition.h. * In postgresGetForeignModifyBatchSize(): /* -* Should never get called when the insert is being performed as part of a -* row movement operation. +* Use the auxiliary state if any; see postgresBeginForeignInsert() for +* details on what it represents. */ - Assert(fmstate == NULL || fmstate->aux_fmstate == NULL); + if (fmstate != NULL && fmstate->aux_fmstate != NULL) + fmstate = fmstate->aux_fmstate; I might be missing something, but I think we should leave the Assert as-is, because we still disallow to move rows to another foreign-table partition that is also an UPDATE subplan result relation, which means that any fmstate should have fmstate->aux_fmstate=NULL. * Also in that function: - if (fmstate) + if (fmstate != NULL) This is correct, but I would vote for leaving that as-is, to make back-patching easy. That is all I have for now. I will mark this as Waiting on Author. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/CAPmGK17rmXEY3BL%3DAq71L8UZv5f-mz%3DuxJkz1kMnfSSY%2BpFe-A%40mail.gmail.com
Re: Allow batched insert during cross-partition updates
On Tue, Mar 22, 2022 at 9:30 AM Andres Freund wrote: > On 2021-08-24 12:03:59 +0900, Amit Langote wrote: > > Tomas committed the bug-fix, so attaching a rebased version of the > > patch for $subject. > > This patch is in the current CF, but doesn't apply: > http://cfbot.cputube.org/patch_37_2992.log > marked the entry as waiting-on-author. Thanks for the heads up. Rebased to fix a minor conflict with some recently committed nodeModifyTable.c changes. -- Amit Langote EDB: http://www.enterprisedb.com v10-0001-Allow-batching-of-inserts-during-cross-partition.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
Hi, On 2021-08-24 12:03:59 +0900, Amit Langote wrote: > Tomas committed the bug-fix, so attaching a rebased version of the > patch for $subject. This patch is in the current CF, but doesn't apply: http://cfbot.cputube.org/patch_37_2992.log marked the entry as waiting-on-author. Greetings, Andres Freund
Re: Allow batched insert during cross-partition updates
On Tue, Jul 27, 2021 at 11:32 AM Amit Langote wrote: > On Thu, Jul 22, 2021 at 2:18 PM vignesh C wrote: > > One of the test postgres_fdw has failed, can you please post an > > updated patch for the fix: > > test postgres_fdw ... FAILED (test process exited with > > exit code 2) 7264 ms > > Thanks Vignesh. > > I found a problem with the underlying batching code that caused this > failure and have just reported it here: > > https://www.postgresql.org/message-id/CA%2BHiwqEWd5B0-e-RvixGGUrNvGkjH2s4m95%3DJcwUnyV%3Df0rAKQ%40mail.gmail.com > > Here's v8, including the patch for the above problem. Tomas committed the bug-fix, so attaching a rebased version of the patch for $subject. -- Amit Langote EDB: http://www.enterprisedb.com v9-0001-Allow-batching-of-inserts-during-cross-partition-.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
On Thu, Jul 22, 2021 at 2:18 PM vignesh C wrote: > On Fri, Jul 2, 2021 at 7:35 AM Amit Langote wrote: > > On Fri, Jul 2, 2021 at 1:39 AM Tom Lane wrote: > > > > > > Amit Langote writes: > > > > [ v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch ] > > > > > > Per the cfbot, this isn't applying anymore, so I'm setting it back > > > to Waiting on Author. > > > > Rebased patch attached. Thanks for the reminder. > > One of the test postgres_fdw has failed, can you please post an > updated patch for the fix: > test postgres_fdw ... FAILED (test process exited with > exit code 2) 7264 ms Thanks Vignesh. I found a problem with the underlying batching code that caused this failure and have just reported it here: https://www.postgresql.org/message-id/CA%2BHiwqEWd5B0-e-RvixGGUrNvGkjH2s4m95%3DJcwUnyV%3Df0rAKQ%40mail.gmail.com Here's v8, including the patch for the above problem. -- Amit Langote EDB: http://www.enterprisedb.com v8-0002-Allow-batching-of-inserts-during-cross-partition-.patch Description: Binary data v8-0001-Fix-a-thinko-in-b676ac443b6.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
On Fri, Jul 2, 2021 at 7:35 AM Amit Langote wrote: > > On Fri, Jul 2, 2021 at 1:39 AM Tom Lane wrote: > > > > Amit Langote writes: > > > [ v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch ] > > > > Per the cfbot, this isn't applying anymore, so I'm setting it back > > to Waiting on Author. > > Rebased patch attached. Thanks for the reminder. One of the test postgres_fdw has failed, can you please post an updated patch for the fix: test postgres_fdw ... FAILED (test process exited with exit code 2) 7264 ms Regards, Vignesh
Re: Allow batched insert during cross-partition updates
On Fri, Jul 2, 2021 at 1:39 AM Tom Lane wrote: > > Amit Langote writes: > > [ v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch ] > > Per the cfbot, this isn't applying anymore, so I'm setting it back > to Waiting on Author. Rebased patch attached. Thanks for the reminder. -- Amit Langote EDB: http://www.enterprisedb.com v7-0001-Allow-batching-of-inserts-during-cross-partition-.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
Amit Langote writes: > [ v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch ] Per the cfbot, this isn't applying anymore, so I'm setting it back to Waiting on Author. regards, tom lane
Re: Allow batched insert during cross-partition updates
On Fri, May 7, 2021 at 6:39 PM houzj.f...@fujitsu.com
wrote:
>
> > > 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.
Thanks, done.
> 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.
That's a good observation. Fixed.
> 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
> &&..."
I don't quite remember why I added that test, because nowhere do we
add a NULL value to es_opened_result_relations. Actually, we can even
Assert(resultRelInfo != NULL) here.
> 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)".
Sure, done. Actually, there's a if (fmstate) statement right below
the code being added, which I fixed to match the style used by the new
code.
--
Amit Langote
EDB: http://www.enterprisedb.com
v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch
Description: Binary data
RE: Allow batched insert during cross-partition updates
> > 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
Re: Allow batched insert during cross-partition updates
On Tue, Apr 6, 2021 at 10:52 PM Bharath Rupireddy wrote: > > On Tue, Apr 6, 2021 at 6:37 PM Amit Langote wrote: > > > 3) will the cmin in the output always be the same? > > > +SELECT cmin, * FROM batch_cp_upd_test3; > > > > TBH, I am not so sure. Maybe it's not a good idea to rely on cmin > > after all. I've rewritten the tests to use a different method of > > determining if a single or multiple insert commands were used in > > moving rows into foreign partitions. > > Thanks! It looks good! Thanks for checking. I'll mark this as RfC. -- Amit Langote EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
On Tue, Apr 6, 2021 at 6:37 PM Amit Langote wrote: > > 3) will the cmin in the output always be the same? > > +SELECT cmin, * FROM batch_cp_upd_test3; > > TBH, I am not so sure. Maybe it's not a good idea to rely on cmin > after all. I've rewritten the tests to use a different method of > determining if a single or multiple insert commands were used in > moving rows into foreign partitions. Thanks! It looks good! With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
On Tue, Apr 6, 2021 at 6:49 PM Bharath Rupireddy wrote: > On Tue, Apr 6, 2021 at 3:08 PM Amit Langote wrote: > > Updated patch attached. I had to adjust the test case a little bit to > > account for the changes of 86dc90056d, something I failed to notice > > yesterday. Also, I expanded the test case added in postgres_fdw.sql a > > bit to show the batching in action more explicitly. > > Some minor comments: Thanks for the review. > 1) don't we need order by clause for the selects in the tests added? > +SELECT tableoid::regclass, * FROM batch_cp_upd_test; Good point. It wasn't necessary before, but it is after the test expansion, so added. > 3) will the cmin in the output always be the same? > +SELECT cmin, * FROM batch_cp_upd_test3; TBH, I am not so sure. Maybe it's not a good idea to rely on cmin after all. I've rewritten the tests to use a different method of determining if a single or multiple insert commands were used in moving rows into foreign partitions. -- Amit Langote EDB: http://www.enterprisedb.com v5-0001-Allow-batching-of-inserts-during-cross-partition-.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
On Tue, Apr 6, 2021 at 3:08 PM Amit Langote wrote: > > On Mon, Apr 5, 2021 at 1:16 AM Zhihong Yu wrote: > > > > Hi, > > In the description: > > > > cross-partition update of partitioned tables can't use batching > > because ExecInitRoutingInfo() which initializes the insert target > > > > 'which' should be dropped since 'because' should start a sentence. > > > > +-- Check that batched inserts also works for inserts made during > > > > inserts also works -> inserts also work > > > > + Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind == > > + RELKIND_PARTITIONED_TABLE); > > > > The level of nested field accesses is quite deep. If the assertion fails, > > it would be hard to know which field is null. > > Maybe use several assertions: > >Assert(node->rootResultRelInfo) > >Assert(node->rootResultRelInfo->ri_RelationDesc) > >Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind == > > ... > > Thanks for taking a look at this. > > While I agree about having the 1st Assert you suggest, I don't think > this code needs the 2nd one, because its failure would very likely be > due to a problem in some totally unrelated code. > > Updated patch attached. I had to adjust the test case a little bit to > account for the changes of 86dc90056d, something I failed to notice > yesterday. Also, I expanded the test case added in postgres_fdw.sql a > bit to show the batching in action more explicitly. Some minor comments: 1) don't we need order by clause for the selects in the tests added? +SELECT tableoid::regclass, * FROM batch_cp_upd_test; +SELECT cmin, * FROM batch_cp_upd_test1; 2) typo - it is "should" not "shoud" +-- cmin shoud be different across rows, because each one would be inserted 3) will the cmin in the output always be the same? +SELECT cmin, * FROM batch_cp_upd_test3; With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
On Mon, Apr 5, 2021 at 1:16 AM Zhihong Yu wrote: > > Hi, > In the description: > > cross-partition update of partitioned tables can't use batching > because ExecInitRoutingInfo() which initializes the insert target > > 'which' should be dropped since 'because' should start a sentence. > > +-- Check that batched inserts also works for inserts made during > > inserts also works -> inserts also work > > + Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind == > + RELKIND_PARTITIONED_TABLE); > > The level of nested field accesses is quite deep. If the assertion fails, it > would be hard to know which field is null. > Maybe use several assertions: >Assert(node->rootResultRelInfo) >Assert(node->rootResultRelInfo->ri_RelationDesc) >Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind == ... Thanks for taking a look at this. While I agree about having the 1st Assert you suggest, I don't think this code needs the 2nd one, because its failure would very likely be due to a problem in some totally unrelated code. Updated patch attached. I had to adjust the test case a little bit to account for the changes of 86dc90056d, something I failed to notice yesterday. Also, I expanded the test case added in postgres_fdw.sql a bit to show the batching in action more explicitly. -- Amit Langote EDB: http://www.enterprisedb.com v4-0001-Allow-batching-of-inserts-during-cross-partition-.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
Hi, In the description: cross-partition update of partitioned tables can't use batching because ExecInitRoutingInfo() which initializes the insert target 'which' should be dropped since 'because' should start a sentence. +-- Check that batched inserts also works for inserts made during inserts also works -> inserts also work + Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind == + RELKIND_PARTITIONED_TABLE); The level of nested field accesses is quite deep. If the assertion fails, it would be hard to know which field is null. Maybe use several assertions: Assert(node->rootResultRelInfo) Assert(node->rootResultRelInfo->ri_RelationDesc) Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind == ... Cheers On Sun, Apr 4, 2021 at 8:06 AM Amit Langote wrote: > On Tue, Mar 16, 2021 at 6:13 PM wrote: > > Status updated to RfC in the commitfest app. > > Patch fails to apply per cfbot, so rebased. > > -- > Amit Langote > EDB: http://www.enterprisedb.com >
Re: Allow batched insert during cross-partition updates
On Tue, Mar 16, 2021 at 6:13 PM wrote: > Status updated to RfC in the commitfest app. Patch fails to apply per cfbot, so rebased. -- Amit Langote EDB: http://www.enterprisedb.com v3-0001-Allow-batching-of-inserts-during-cross-partition-.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
‐‐‐ Original Message ‐‐‐ On Tuesday, March 16, 2021 9:59 AM, Amit Langote wrote: > Hi Georgios, > > On Tue, Mar 16, 2021 at 5:12 PM gkokola...@pm.me wrote: > > > On Tuesday, March 16, 2021 6:13 AM, Amit Langote amitlangot...@gmail.com > > wrote: > > > > > On Fri, Mar 12, 2021 at 7:59 PM gkokola...@pm.me wrote: > > > > > > > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangot...@gmail.com > > > > wrote: > > > > > > > > > By the way, the test case added by commit 927f453a94106 does exercise > > > > > the code added by this patch, but as I said in the last paragraph, we > > > > > can't verify that by inspecting EXPLAIN output. > > > > > > > > I never doubted that. However, there is a difference. The current patch > > > > changes the query to be executed in the remote from: > > > > INSERT INTO VALUES ($1); > > > > to: > > > > INSERT INTO VALUES ($1), ($2) ... ($n); > > > > When this patch gets in, it would be very helpful to know that > > > > subsequent > > > > code changes will not cause regressions. So I was wondering if there is > > > > a way to craft a test case that would break for the code in > > > > 927f453a94106 > > > > yet succeed with the current patch. > > > > > > The test case "works" both before and after the patch, with the > > > difference being in the form of the remote query. It seems to me > > > though that you do get that. > > > > > > > I attach version 2 of my small reproduction. I am under the impression > > > > that > > > > in this version, examining the value of cmin in the remote table should > > > > give an indication of whether the remote received a multiple insert > > > > queries > > > > with a single value, or a single insert query with multiple values. > > > > Or is this a wrong assumption of mine? > > > > > > No, I think you have a good idea here. > > > I've adjusted that test case to confirm that the batching indeed works > > > by checking cmin of the moved rows, as you suggest. Please check the > > > attached updated patch. > > > > Excellent. The patch in the current version with the added test seems > > ready to me. > > Thanks for quickly checking that. A pleasure. > > > I would still vote to have accessor functions instead of exposing the > > whole PartitionTupleRouting struct, but I am not going to hold a too > > strong stance about it. > > I as well, although I would wait for others to chime in before > updating the patch that way. Fair enough. Status updated to RfC in the commitfest app. > > -- > > Amit Langote > EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
Hi Georgios, On Tue, Mar 16, 2021 at 5:12 PM wrote: > On Tuesday, March 16, 2021 6:13 AM, Amit Langote > wrote: > > On Fri, Mar 12, 2021 at 7:59 PM gkokola...@pm.me wrote: > > > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangot...@gmail.com > > > wrote: > > > > By the way, the test case added by commit 927f453a94106 does exercise > > > > the code added by this patch, but as I said in the last paragraph, we > > > > can't verify that by inspecting EXPLAIN output. > > > > > > I never doubted that. However, there is a difference. The current patch > > > changes the query to be executed in the remote from: > > > INSERT INTO VALUES ($1); > > > to: > > > INSERT INTO VALUES ($1), ($2) ... ($n); > > > When this patch gets in, it would be very helpful to know that subsequent > > > code changes will not cause regressions. So I was wondering if there is > > > a way to craft a test case that would break for the code in 927f453a94106 > > > yet succeed with the current patch. > > > > The test case "works" both before and after the patch, with the > > difference being in the form of the remote query. It seems to me > > though that you do get that. > > > > > I attach version 2 of my small reproduction. I am under the impression > > > that > > > in this version, examining the value of cmin in the remote table should > > > give an indication of whether the remote received a multiple insert > > > queries > > > with a single value, or a single insert query with multiple values. > > > Or is this a wrong assumption of mine? > > > > No, I think you have a good idea here. > > > > I've adjusted that test case to confirm that the batching indeed works > > by checking cmin of the moved rows, as you suggest. Please check the > > attached updated patch. > > Excellent. The patch in the current version with the added test seems > ready to me. Thanks for quickly checking that. > I would still vote to have accessor functions instead of exposing the > whole PartitionTupleRouting struct, but I am not going to hold a too > strong stance about it. I as well, although I would wait for others to chime in before updating the patch that way. -- Amit Langote EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
‐‐‐ Original Message ‐‐‐ On Tuesday, March 16, 2021 6:13 AM, Amit Langote wrote: > Hi Georgios, > > On Fri, Mar 12, 2021 at 7:59 PM gkokola...@pm.me wrote: > > > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangot...@gmail.com > > wrote: > > > > > On Thu, Mar 11, 2021 at 8:36 PM gkokola...@pm.me wrote: > > > > > > > On Thursday, March 11, 2021 9:42 AM, Amit Langote > > > > amitlangot...@gmail.com wrote: > > > > > > > > > What we do support however is moving rows from a local partition to a > > > > > remote partition and that involves performing an INSERT on the latter. > > > > > This patch is for teaching those INSERTs to use batched mode if > > > > > allowed, which is currently prohibited. So with this patch, if an > > > > > UPDATE moves 10 rows from a local partition to a remote partition, > > > > > then they will be inserted with a single INSERT command containing all > > > > > 10 rows, instead of 10 separate INSERT commands. > > > > > > > > So, if I understand correctly then in my previously attached repro I > > > > should have written instead: > > > > > > > > CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST > > > > ( a ); > > > > CREATE TABLE > > > > local_root_local_partition_1 > > > > PARTITION OF > > > > local_root_remote_partitions FOR VALUES IN (1); > > > > > > > > CREATE FOREIGN TABLE > > > > local_root_remote_partition_2 > > > > PARTITION OF > > > > local_root_remote_partitions FOR VALUES IN (2) > > > > SERVER > > > > remote_server > > > > OPTIONS ( > > > > table_name 'remote_partition_2', > > > > batch_size '10' > > > > ); > > > > > > > > INSERT INTO local_root_remote_partitions VALUES (1), (1); > > > > -- Everything should be on local_root_local_partition_1 and on the > > > > same transaction > > > > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM > > > > local_root_remote_partitions; > > > > > > > > UPDATE local_root_remote_partitions SET a = 2; > > > > -- Everything should be on remote_partition_2 and on the same > > > > transaction > > > > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM > > > > local_root_remote_partitions; > > > > > > > > > > > > I am guessing that I am still wrong because the UPDATE operation above > > > > will > > > > fail due to the restrictions imposed in postgresBeginForeignInsert > > > > regarding > > > > UPDATES. > > > > > > Yeah, for the move to work without hitting the restriction you > > > mention, you will need to write the UPDATE query such that > > > local_root_remote_partition_2 is not updated. For example, as > > > follows: > > > UPDATE local_root_remote_partitions SET a = 2 WHERE a <> 2; > > > With this query, the remote partition is not one of the result > > > relations to be updated, so is able to escape that restriction. > > > > Excellent. Thank you for the explanation and patience. > > > > > > Would it be too much to ask for the addition of a test case that will > > > > demonstrate the change of behaviour found in patch. > > > > > > Hmm, I don't think there's a way to display whether the INSERT done on > > > the remote partition as a part of an (tuple-moving) UPDATE used > > > batching or not. That's because that INSERT's state is hidden from > > > EXPLAIN. Maybe we should change EXPLAIN to make it show such hidden > > > INSERT's state (especially its batch size) under the original UPDATE > > > node, but I am not sure. > > > > Yeah, there does not seem to be a way for explain to do show that > > information > > with the current code. > > > > > By the way, the test case added by commit 927f453a94106 does exercise > > > the code added by this patch, but as I said in the last paragraph, we > > > can't verify that by inspecting EXPLAIN output. > > > > I never doubted that. However, there is a difference. The current patch > > changes the query to be executed in the remote from: > > INSERT INTO VALUES ($1); > > to: > > INSERT INTO VALUES ($1), ($2) ... ($n); > > When this patch gets in, it would be very helpful to know that subsequent > > code changes will not cause regressions. So I was wondering if there is > > a way to craft a test case that would break for the code in 927f453a94106 > > yet succeed with the current patch. > > The test case "works" both before and after the patch, with the > difference being in the form of the remote query. It seems to me > though that you do get that. > > > I attach version 2 of my small reproduction. I am under the impression that > > in this version, examining the value of cmin in the remote table should > > give an indication of whether the remote received a multiple insert queries > > with a single value, or a single insert query with multiple values. > > Or is this a wrong assumption of mine? > > No, I think you have a good idea here. Thank you. > > I've adjusted that test case to confirm that the batching indeed works > by
Re: Allow batched insert during cross-partition updates
Hi Georgios, On Fri, Mar 12, 2021 at 7:59 PM wrote: > On Friday, March 12, 2021 3:45 AM, Amit Langote > wrote: > > On Thu, Mar 11, 2021 at 8:36 PM gkokola...@pm.me wrote: > > > On Thursday, March 11, 2021 9:42 AM, Amit Langote amitlangot...@gmail.com > > > wrote: > > > > What we do support however is moving rows from a local partition to a > > > > remote partition and that involves performing an INSERT on the latter. > > > > This patch is for teaching those INSERTs to use batched mode if > > > > allowed, which is currently prohibited. So with this patch, if an > > > > UPDATE moves 10 rows from a local partition to a remote partition, > > > > then they will be inserted with a single INSERT command containing all > > > > 10 rows, instead of 10 separate INSERT commands. > > > > > > So, if I understand correctly then in my previously attached repro I > > > should have written instead: > > > > > > CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST ( > > > a ); > > > CREATE TABLE > > > local_root_local_partition_1 > > > PARTITION OF > > > local_root_remote_partitions FOR VALUES IN (1); > > > > > > CREATE FOREIGN TABLE > > > local_root_remote_partition_2 > > > PARTITION OF > > > local_root_remote_partitions FOR VALUES IN (2) > > > SERVER > > > remote_server > > > OPTIONS ( > > > table_name 'remote_partition_2', > > > batch_size '10' > > > ); > > > > > > INSERT INTO local_root_remote_partitions VALUES (1), (1); > > > -- Everything should be on local_root_local_partition_1 and on the > > > same transaction > > > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM > > > local_root_remote_partitions; > > > > > > UPDATE local_root_remote_partitions SET a = 2; > > > -- Everything should be on remote_partition_2 and on the same > > > transaction > > > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM > > > local_root_remote_partitions; > > > > > > > > > I am guessing that I am still wrong because the UPDATE operation above > > > will > > > fail due to the restrictions imposed in postgresBeginForeignInsert > > > regarding > > > UPDATES. > > > > Yeah, for the move to work without hitting the restriction you > > mention, you will need to write the UPDATE query such that > > local_root_remote_partition_2 is not updated. For example, as > > follows: > > > > UPDATE local_root_remote_partitions SET a = 2 WHERE a <> 2; > > > > With this query, the remote partition is not one of the result > > relations to be updated, so is able to escape that restriction. > > Excellent. Thank you for the explanation and patience. > > > > Would it be too much to ask for the addition of a test case that will > > > demonstrate the change of behaviour found in patch. > > > > Hmm, I don't think there's a way to display whether the INSERT done on > > the remote partition as a part of an (tuple-moving) UPDATE used > > batching or not. That's because that INSERT's state is hidden from > > EXPLAIN. Maybe we should change EXPLAIN to make it show such hidden > > INSERT's state (especially its batch size) under the original UPDATE > > node, but I am not sure. > > Yeah, there does not seem to be a way for explain to do show that information > with the current code. > > > By the way, the test case added by commit 927f453a94106 does exercise > > the code added by this patch, but as I said in the last paragraph, we > > can't verify that by inspecting EXPLAIN output. > > I never doubted that. However, there is a difference. The current patch > changes the query to be executed in the remote from: > >INSERT INTO VALUES ($1); > to: >INSERT INTO VALUES ($1), ($2) ... ($n); > > When this patch gets in, it would be very helpful to know that subsequent > code changes will not cause regressions. So I was wondering if there is > a way to craft a test case that would break for the code in 927f453a94106 > yet succeed with the current patch. The test case "works" both before and after the patch, with the difference being in the form of the remote query. It seems to me though that you do get that. > I attach version 2 of my small reproduction. I am under the impression that > in this version, examining the value of cmin in the remote table should > give an indication of whether the remote received a multiple insert queries > with a single value, or a single insert query with multiple values. > > Or is this a wrong assumption of mine? No, I think you have a good idea here. I've adjusted that test case to confirm that the batching indeed works by checking cmin of the moved rows, as you suggest. Please check the attached updated patch. -- Amit Langote EDB: http://www.enterprisedb.com v2-0001-Allow-batching-of-inserts-during-cross-partition-.patch Description: Binary data
Re: Allow batched insert during cross-partition updates
‐‐‐ Original Message ‐‐‐ On Friday, March 12, 2021 3:45 AM, Amit Langote wrote: > On Thu, Mar 11, 2021 at 8:36 PM gkokola...@pm.me wrote: > > > On Thursday, March 11, 2021 9:42 AM, Amit Langote amitlangot...@gmail.com > > wrote: > > > > > On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokola...@protonmail.com wrote: > > > > > > > I continued looking a bit at the patch, yet I am either failing to see > > > > fix or I am > > > > looking at the wrong thing. Please find attached a small repro of what > > > > my expectetions > > > > were. > > > > As you can see in the repro, I would expect the > > > > UPDATE local_root_remote_partitions SET a = 2; > > > > to move the tuples to remote_partition_2 on the same transaction. > > > > However this is not the case, with or without the patch. > > > > Is my expectation of this patch wrong? > > > > > > I think yes. We currently don't have the feature you are looking for > > > -- moving tuples from one remote partition to another remote > > > partition. This patch is not for adding that feature. > > > > Thank you for correcting me. > > > > > What we do support however is moving rows from a local partition to a > > > remote partition and that involves performing an INSERT on the latter. > > > This patch is for teaching those INSERTs to use batched mode if > > > allowed, which is currently prohibited. So with this patch, if an > > > UPDATE moves 10 rows from a local partition to a remote partition, > > > then they will be inserted with a single INSERT command containing all > > > 10 rows, instead of 10 separate INSERT commands. > > > > So, if I understand correctly then in my previously attached repro I > > should have written instead: > > > > CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST ( a > > ); > > CREATE TABLE > > local_root_local_partition_1 > > PARTITION OF > > local_root_remote_partitions FOR VALUES IN (1); > > > > CREATE FOREIGN TABLE > > local_root_remote_partition_2 > > PARTITION OF > > local_root_remote_partitions FOR VALUES IN (2) > > SERVER > > remote_server > > OPTIONS ( > > table_name 'remote_partition_2', > > batch_size '10' > > ); > > > > INSERT INTO local_root_remote_partitions VALUES (1), (1); > > -- Everything should be on local_root_local_partition_1 and on the same > > transaction > > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM > > local_root_remote_partitions; > > > > UPDATE local_root_remote_partitions SET a = 2; > > -- Everything should be on remote_partition_2 and on the same > > transaction > > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM > > local_root_remote_partitions; > > > > > > I am guessing that I am still wrong because the UPDATE operation above will > > fail due to the restrictions imposed in postgresBeginForeignInsert regarding > > UPDATES. > > Yeah, for the move to work without hitting the restriction you > mention, you will need to write the UPDATE query such that > local_root_remote_partition_2 is not updated. For example, as > follows: > > UPDATE local_root_remote_partitions SET a = 2 WHERE a <> 2; > > With this query, the remote partition is not one of the result > relations to be updated, so is able to escape that restriction. Excellent. Thank you for the explanation and patience. > > > Would it be too much to ask for the addition of a test case that will > > demonstrate the change of behaviour found in patch. > > Hmm, I don't think there's a way to display whether the INSERT done on > the remote partition as a part of an (tuple-moving) UPDATE used > batching or not. That's because that INSERT's state is hidden from > EXPLAIN. Maybe we should change EXPLAIN to make it show such hidden > INSERT's state (especially its batch size) under the original UPDATE > node, but I am not sure. Yeah, there does not seem to be a way for explain to do show that information with the current code. > > By the way, the test case added by commit 927f453a94106 does exercise > the code added by this patch, but as I said in the last paragraph, we > can't verify that by inspecting EXPLAIN output. I never doubted that. However, there is a difference. The current patch changes the query to be executed in the remote from: INSERT INTO VALUES ($1); to: INSERT INTO VALUES ($1), ($2) ... ($n); When this patch gets in, it would be very helpful to know that subsequent code changes will not cause regressions. So I was wondering if there is a way to craft a test case that would break for the code in 927f453a94106 yet succeed with the current patch. I attach version 2 of my small reproduction. I am under the impression that in this version, examining the value of cmin in the remote table should give an indication of whether the remote received a multiple insert queries with a single value, or a single insert query with multiple values. Or is this a wrong assumption of mine?
Re: Allow batched insert during cross-partition updates
On Thu, Mar 11, 2021 at 8:36 PM wrote: > On Thursday, March 11, 2021 9:42 AM, Amit Langote > wrote: > > On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokola...@protonmail.com wrote: > > > > > I continued looking a bit at the patch, yet I am either failing to see > > > fix or I am > > > looking at the wrong thing. Please find attached a small repro of what my > > > expectetions > > > were. > > > As you can see in the repro, I would expect the > > > UPDATE local_root_remote_partitions SET a = 2; > > > to move the tuples to remote_partition_2 on the same transaction. > > > However this is not the case, with or without the patch. > > > Is my expectation of this patch wrong? > > > > I think yes. We currently don't have the feature you are looking for > > -- moving tuples from one remote partition to another remote > > partition. This patch is not for adding that feature. > > Thank you for correcting me. > > > > What we do support however is moving rows from a local partition to a > > remote partition and that involves performing an INSERT on the latter. > > This patch is for teaching those INSERTs to use batched mode if > > allowed, which is currently prohibited. So with this patch, if an > > UPDATE moves 10 rows from a local partition to a remote partition, > > then they will be inserted with a single INSERT command containing all > > 10 rows, instead of 10 separate INSERT commands. > > So, if I understand correctly then in my previously attached repro I > should have written instead: > > CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST ( a ); > CREATE TABLE > local_root_local_partition_1 > PARTITION OF > local_root_remote_partitions FOR VALUES IN (1); > > CREATE FOREIGN TABLE > local_root_remote_partition_2 > PARTITION OF > local_root_remote_partitions FOR VALUES IN (2) > SERVER > remote_server > OPTIONS ( > table_name 'remote_partition_2', > batch_size '10' > ); > > INSERT INTO local_root_remote_partitions VALUES (1), (1); > -- Everything should be on local_root_local_partition_1 and on the same > transaction > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM > local_root_remote_partitions; > > UPDATE local_root_remote_partitions SET a = 2; > -- Everything should be on remote_partition_2 and on the same transaction > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM > local_root_remote_partitions; > > > I am guessing that I am still wrong because the UPDATE operation above will > fail due to the restrictions imposed in postgresBeginForeignInsert regarding > UPDATES. Yeah, for the move to work without hitting the restriction you mention, you will need to write the UPDATE query such that local_root_remote_partition_2 is not updated. For example, as follows: UPDATE local_root_remote_partitions SET a = 2 WHERE a <> 2; With this query, the remote partition is not one of the result relations to be updated, so is able to escape that restriction. > Would it be too much to ask for the addition of a test case that will > demonstrate the change of behaviour found in patch. Hmm, I don't think there's a way to display whether the INSERT done on the remote partition as a part of an (tuple-moving) UPDATE used batching or not. That's because that INSERT's state is hidden from EXPLAIN. Maybe we should change EXPLAIN to make it show such hidden INSERT's state (especially its batch size) under the original UPDATE node, but I am not sure. By the way, the test case added by commit 927f453a94106 does exercise the code added by this patch, but as I said in the last paragraph, we can't verify that by inspecting EXPLAIN output. -- Amit Langote EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
‐‐‐ Original Message ‐‐‐ On Thursday, March 11, 2021 9:42 AM, Amit Langote wrote: > Hi Georgios, > > On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokola...@protonmail.com wrote: > > > I continued looking a bit at the patch, yet I am either failing to see fix > > or I am > > looking at the wrong thing. Please find attached a small repro of what my > > expectetions > > were. > > As you can see in the repro, I would expect the > > UPDATE local_root_remote_partitions SET a = 2; > > to move the tuples to remote_partition_2 on the same transaction. > > However this is not the case, with or without the patch. > > Is my expectation of this patch wrong? > > I think yes. We currently don't have the feature you are looking for > -- moving tuples from one remote partition to another remote > partition. This patch is not for adding that feature. Thank you for correcting me. > > What we do support however is moving rows from a local partition to a > remote partition and that involves performing an INSERT on the latter. > This patch is for teaching those INSERTs to use batched mode if > allowed, which is currently prohibited. So with this patch, if an > UPDATE moves 10 rows from a local partition to a remote partition, > then they will be inserted with a single INSERT command containing all > 10 rows, instead of 10 separate INSERT commands. So, if I understand correctly then in my previously attached repro I should have written instead: CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST ( a ); CREATE TABLE local_root_local_partition_1 PARTITION OF local_root_remote_partitions FOR VALUES IN (1); CREATE FOREIGN TABLE local_root_remote_partition_2 PARTITION OF local_root_remote_partitions FOR VALUES IN (2) SERVER remote_server OPTIONS ( table_name 'remote_partition_2', batch_size '10' ); INSERT INTO local_root_remote_partitions VALUES (1), (1); -- Everything should be on local_root_local_partition_1 and on the same transaction SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM local_root_remote_partitions; UPDATE local_root_remote_partitions SET a = 2; -- Everything should be on remote_partition_2 and on the same transaction SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM local_root_remote_partitions; I am guessing that I am still wrong because the UPDATE operation above will fail due to the restrictions imposed in postgresBeginForeignInsert regarding UPDATES. Would it be too much to ask for the addition of a test case that will demonstrate the change of behaviour found in patch? Cheers, //Georgios > > - > > Amit Langote > EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
Hi Georgios, On Wed, Mar 10, 2021 at 9:30 PM Georgios wrote: > I continued looking a bit at the patch, yet I am either failing to see fix or > I am > looking at the wrong thing. Please find attached a small repro of what my > expectetions > were. > > As you can see in the repro, I would expect the > UPDATE local_root_remote_partitions SET a = 2; > to move the tuples to remote_partition_2 on the same transaction. > However this is not the case, with or without the patch. > > Is my expectation of this patch wrong? I think yes. We currently don't have the feature you are looking for -- moving tuples from one remote partition to another remote partition. This patch is not for adding that feature. What we do support however is moving rows from a local partition to a remote partition and that involves performing an INSERT on the latter. This patch is for teaching those INSERTs to use batched mode if allowed, which is currently prohibited. So with this patch, if an UPDATE moves 10 rows from a local partition to a remote partition, then they will be inserted with a single INSERT command containing all 10 rows, instead of 10 separate INSERT commands. -- Amit Langote EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
Hi Georgios, On Wed, Mar 10, 2021 at 12:54 AM Georgios Kokolatos wrote: > > Hi, > > thanks for the patch. I had a first look and played around with the code. > > The code seems clean, complete, and does what it says on the tin. I will > need a bit more time to acclimatise with all the use cases for a more > thorough review. Thanks for checking. > I small question though is why expose PartitionTupleRouting and not add > a couple of functions to get the necessary info? If I have read the code > correctly the only members actually needed to be exposed are num_partitions > and partitions. Not a critique, I am just curious. I had implemented accessor functions in an earlier unposted version of the patch, but just exposing PartitionTupleRouting does not sound so harmful, so I switched to that approach. Maybe if others agree with you that accessor functions would be better, I will change the patch that way. -- Amit Langote EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
‐‐‐ Original Message ‐‐‐ On Wednesday, March 10, 2021 1:23 PM, Georgios wrote: > > > Hi, > > ‐‐‐ Original Message ‐‐‐ > On Thursday, February 18, 2021 10:54 AM, Amit Langote amitlangot...@gmail.com > wrote: > > > On Thu, Feb 18, 2021 at 6:52 PM Amit Langote amitlangot...@gmail.com wrote: > > > > > Based on the discussion at: > > > https://www.postgresql.org/message-id/6929d485-2d2a-da46-3681-4a400a3d794f%40enterprisedb.com > > > I'm posting the patch for $subject here in this new thread and I'll > > > add it to the next CF per Tomas' advice. > > > > Done:https://commitfest.postgresql.org/32/2992/ > > apparently I did not receive the review comment I sent via the commitfest app. > Apologies for the chatter. Find the message-id here: https://www.postgresql.org/message-id/161530518971.29967.9368488207318158252.pgcf%40coridan.postgresql.org I continued looking a bit at the patch, yet I am either failing to see fix or I am looking at the wrong thing. Please find attached a small repro of what my expectetions were. As you can see in the repro, I would expect the UPDATE local_root_remote_partitions SET a = 2; to move the tuples to remote_partition_2 on the same transaction. However this is not the case, with or without the patch. Is my expectation of this patch wrong? Cheers, //Georgios > > > Amit Langote > > EDB: http://www.enterprisedb.com repro.sql Description: application/sql
Re: Allow batched insert during cross-partition updates
Hi, ‐‐‐ Original Message ‐‐‐ On Thursday, February 18, 2021 10:54 AM, Amit Langote wrote: > On Thu, Feb 18, 2021 at 6:52 PM Amit Langote amitlangot...@gmail.com wrote: > > > Based on the discussion at: > > https://www.postgresql.org/message-id/6929d485-2d2a-da46-3681-4a400a3d794f%40enterprisedb.com > > I'm posting the patch for $subject here in this new thread and I'll > > add it to the next CF per Tomas' advice. > > Done:https://commitfest.postgresql.org/32/2992/ > > -- apparently I did not receive the review comment I sent via the commitfest app. Apologies for the chatter. Find the message-id here: > > Amit Langote > EDB: http://www.enterprisedb.com
Re: Allow batched insert during cross-partition updates
Hi, thanks for the patch. I had a first look and played around with the code. The code seems clean, complete, and does what it says on the tin. I will need a bit more time to acclimatise with all the use cases for a more thorough review. I small question though is why expose PartitionTupleRouting and not add a couple of functions to get the necessary info? If I have read the code correctly the only members actually needed to be exposed are num_partitions and partitions. Not a critique, I am just curious. Cheers, //Georgios
Re: Allow batched insert during cross-partition updates
On Thu, Feb 18, 2021 at 6:52 PM Amit Langote wrote: > > Based on the discussion at: > > https://www.postgresql.org/message-id/6929d485-2d2a-da46-3681-4a400a3d794f%40enterprisedb.com > > I'm posting the patch for $subject here in this new thread and I'll > add it to the next CF per Tomas' advice. Done: https://commitfest.postgresql.org/32/2992/ -- Amit Langote EDB: http://www.enterprisedb.com
