Thread: Allow batched insert during cross-partition updates
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. With 927f453a94106 committed earlier today, we limit insert batching only to the cases where the query's main command is also insert, because allowing it to be used in other cases can hit some limitations of the current code. One such case is cross-partition updates of a partitioned table which internally uses insert. postgres_fdw supports some cases where a row is moved from a local partition to a foreign partition. When doing so, the moved row is inserted into the latter, but those inserts can't use batching due to the aforementioned commit. As described in the thread linked above, to make batching possible for those internal inserts, we'd need to make some changes to both the core code and postgres_fdw, which the attached patch implements. Details are in the commit message. -- Amit Langote EDB: http://www.enterprisedb.com
Attachment
On Thu, Feb 18, 2021 at 6:52 PM Amit Langote <amitlangote09@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/ -- Amit Langote EDB: http://www.enterprisedb.com
On Tue, Mar 16, 2021 at 6:13 PM <gkokolatos@pm.me> wrote: > Status updated to RfC in the commitfest app. Patch fails to apply per cfbot, so rebased. -- Amit Langote EDB: http://www.enterprisedb.com
Attachment
Hi,
In the description:
cross-partition update of partitioned tables can't use batching
because ExecInitRoutingInfo() which initializes the insert target
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);
+ 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 <amitlangote09@gmail.com> wrote:
On Tue, Mar 16, 2021 at 6:13 PM <gkokolatos@pm.me> wrote:
> Status updated to RfC in the commitfest app.
Patch fails to apply per cfbot, so rebased.
--
Amit Langote
EDB: http://www.enterprisedb.com
On Mon, Apr 5, 2021 at 1:16 AM Zhihong Yu <zyu@yugabyte.com> 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
Attachment
On Tue, Apr 6, 2021 at 3:08 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Mon, Apr 5, 2021 at 1:16 AM Zhihong Yu <zyu@yugabyte.com> 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
On Tue, Apr 6, 2021 at 6:49 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > On Tue, Apr 6, 2021 at 3:08 PM Amit Langote <amitlangote09@gmail.com> 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
Attachment
On Tue, Apr 6, 2021 at 6:37 PM Amit Langote <amitlangote09@gmail.com> 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
On Tue, Apr 6, 2021 at 10:52 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Tue, Apr 6, 2021 at 6:37 PM Amit Langote <amitlangote09@gmail.com> 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
> > 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
On Fri, May 7, 2021 at 6:39 PM houzj.fnst@fujitsu.com
<houzj.fnst@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
Attachment
Amit Langote <amitlangote09@gmail.com> 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
On Fri, Jul 2, 2021 at 1:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Langote <amitlangote09@gmail.com> 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
Attachment
On Fri, Jul 2, 2021 at 7:35 AM Amit Langote <amitlangote09@gmail.com> wrote: > > On Fri, Jul 2, 2021 at 1:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Amit Langote <amitlangote09@gmail.com> 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
On Thu, Jul 22, 2021 at 2:18 PM vignesh C <vignesh21@gmail.com> wrote: > On Fri, Jul 2, 2021 at 7:35 AM Amit Langote <amitlangote09@gmail.com> wrote: > > On Fri, Jul 2, 2021 at 1:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > > Amit Langote <amitlangote09@gmail.com> 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
Attachment
On Tue, Jul 27, 2021 at 11:32 AM Amit Langote <amitlangote09@gmail.com> wrote: > On Thu, Jul 22, 2021 at 2:18 PM vignesh C <vignesh21@gmail.com> 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
Attachment
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
On Tue, Mar 22, 2022 at 9:30 AM Andres Freund <andres@anarazel.de> 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
Attachment
Amit-san,
On Tue, Mar 22, 2022 at 10:17 AM Amit Langote <amitlangote09@gmail.com> 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
Hi Fujita-san, On Tue, Dec 6, 2022 at 6:47 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > On Tue, Mar 22, 2022 at 10:17 AM Amit Langote <amitlangote09@gmail.com> 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
Attachment
Amit-san, On Thu, Dec 8, 2022 at 5:00 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Dec 6, 2022 at 6:47 PM Etsuro Fujita <etsuro.fujita@gmail.com> 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
On Thu, Dec 8, 2022 at 8:01 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > On Thu, Dec 8, 2022 at 5:00 PM Amit Langote <amitlangote09@gmail.com> 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
Attachment
On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > On Thu, Dec 8, 2022 at 8:01 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > On Thu, Dec 8, 2022 at 5:00 PM Amit Langote <amitlangote09@gmail.com> 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
Hi Amit-san, On Wed, Dec 14, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> 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
On Tue, Dec 20, 2022 at 7:18 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > On Wed, Dec 14, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> 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