Thread: Remove duplicate table scan in logical apply worker and code refactoring
Remove duplicate table scan in logical apply worker and code refactoring
From
"Zhijie Hou (Fujitsu)"
Date:
Hi, When reviewing the code in logical/worker.c, I noticed that when applying a cross-partition update action, it scans the old partition twice. I am attaching the patch 0001 to remove this duplicate table scan. The test shows that it brings noticeable improvement: Steps ----- Pub: create table tab (a int not null, b int); alter table tab replica identity full; insert into tab select 1,generate_series(1, 1000000, 1); Sub: create table tab (a int not null, b int) partition by range (b); create table tab_1 partition of tab for values from (minvalue) to (5000000); create table tab_2 partition of tab for values from (5000000) to (maxvalue); alter table tab replica identity full; Test query: update tab set b = 6000000 where b > 999900; -- UPDATE 100 Results (The time spent by apply worker to apply the all the UPDATEs): Before 14s After 7s ----- Apart from above, I found there are quite a few duplicate codes related to partition handling(e.g. apply_handle_tuple_routing), so I tried to extract some common logic to simplify the codes. Please see 0002 for this refactoring. Best Regards, Hou Zhijie
Attachment
Re: Remove duplicate table scan in logical apply worker and code refactoring
From
Kirill Reshke
Date:
Hi! > When reviewing the code in logical/worker.c, I noticed that when applying a > cross-partition update action, it scans the old partition twice. Nice catch! > -/* > - * Workhorse for apply_handle_update() > - * relinfo is for the relation we're actually updating in > - * (could be a child partition of edata->targetRelInfo) > - */ > -static void > -apply_handle_update_internal(ApplyExecutionData *edata, > - ResultRelInfo *relinfo, > - TupleTableSlot *remoteslot, > - LogicalRepTupleData *newtup, > - Oid localindexoid) > -{ What's the necessity of this change? Can we modify a function in-place instead of removing and rewriting it in the same file? This will reduce diff, making patch a bit more clear. > +/* > + * If the tuple to be modified could not be found, a log message is emitted. > + */ > +static void > +report_tuple_not_found(CmdType cmd, Relation targetrel, bool is_partition) > +{ > + Assert(cmd == CMD_UPDATE || cmd == CMD_DELETE); > + > + /* XXX should this be promoted to ereport(LOG) perhaps? */ > + elog(DEBUG1, > + "logical replication did not find row to be %s in replication target relation%s \"%s\"", > + cmd == CMD_UPDATE ? "updated" : "deleted", > + is_partition ? "'s partition" : "", > + RelationGetRelationName(targetrel)); > +} Encapsulating report logic inside function is ok, but double ternary expression is a bit out of line. I do not see similar places within PostgreSQL, so it probably violates code style.
Re: Remove duplicate table scan in logical apply worker and code refactoring
From
Ashutosh Bapat
Date:
On Thu, Jul 25, 2024 at 5:56 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > > +/* > > + * If the tuple to be modified could not be found, a log message is emitted. > > + */ > > +static void > > +report_tuple_not_found(CmdType cmd, Relation targetrel, bool is_partition) > > +{ > > + Assert(cmd == CMD_UPDATE || cmd == CMD_DELETE); > > + > > + /* XXX should this be promoted to ereport(LOG) perhaps? */ > > + elog(DEBUG1, > > + "logical replication did not find row to be %s in replication target relation%s \"%s\"", > > + cmd == CMD_UPDATE ? "updated" : "deleted", > > + is_partition ? "'s partition" : "", > > + RelationGetRelationName(targetrel)); > > +} > > Encapsulating report logic inside function is ok, but double ternary > expression is a bit out of line. I do not see similar places within > PostgreSQL, > so it probably violates code style. > They it's written, it would make it hard for the translations. See [1] which redirects to [2]. [1] https://www.postgresql.org/docs/current/error-style-guide.html [2] https://www.postgresql.org/docs/current/nls-programmer.html#NLS-GUIDELINES -- Best Wishes, Ashutosh Bapat
On Thu, Jul 25, 2024 at 4:00 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote: > > When reviewing the code in logical/worker.c, I noticed that when applying a > cross-partition update action, it scans the old partition twice. > I am attaching the patch 0001 to remove this duplicate table scan. > > The test shows that it brings noticeable improvement: > > Steps > ----- > Pub: > create table tab (a int not null, b int); > alter table tab replica identity full; > insert into tab select 1,generate_series(1, 1000000, 1); > > Sub: > create table tab (a int not null, b int) partition by range (b); > create table tab_1 partition of tab for values from (minvalue) to (5000000); > create table tab_2 partition of tab for values from (5000000) to (maxvalue); > alter table tab replica identity full; > > > Test query: > update tab set b = 6000000 where b > 999900; -- UPDATE 100 > > Results (The time spent by apply worker to apply the all the UPDATEs): > Before 14s > After 7s > ----- > The idea sounds good to me. BTW, we don't need the following comment in the 0001 patch: We + * don't call apply_handle_delete_internal() here to avoid + * repeating some work already done above to find the + * local tuple in the partition. It is implied by the change and we already follow the same for the update. -- With Regards, Amit Kapila.
On Thu, Jul 25, 2024 at 5:56 PM Kirill Reshke <reshkekirill@gmail.com> wrote: > > > +/* > > + * If the tuple to be modified could not be found, a log message is emitted. > > + */ > > +static void > > +report_tuple_not_found(CmdType cmd, Relation targetrel, bool is_partition) > > +{ > > + Assert(cmd == CMD_UPDATE || cmd == CMD_DELETE); > > + > > + /* XXX should this be promoted to ereport(LOG) perhaps? */ > > + elog(DEBUG1, > > + "logical replication did not find row to be %s in replication target relation%s \"%s\"", > > + cmd == CMD_UPDATE ? "updated" : "deleted", > > + is_partition ? "'s partition" : "", > > + RelationGetRelationName(targetrel)); > > +} > > Encapsulating report logic inside function is ok, > This could even be a separate patch as it is not directly to other parts of the 0002 patch. BTW, the problem statement for 0002 is not explicitly stated like which part of the code we want to optimize by removing duplication. Also, as proposed the name apply_handle_tuple_routing() for the function doesn't seem suitable as it no longer remains similar to other apply_handle_* functions where we perform the required operation like insert or update the tuple. How about naming it as apply_tuple_routing()? -- With Regards, Amit Kapila.
RE: Remove duplicate table scan in logical apply worker and code refactoring
From
"Hayato Kuroda (Fujitsu)"
Date:
Dear Hou, Thanks for creating a patch! > When reviewing the code in logical/worker.c, I noticed that when applying a > cross-partition update action, it scans the old partition twice. > I am attaching the patch 0001 to remove this duplicate table scan. Just to clarify, you meant that FindReplTupleInLocalRel() are called in apply_handle_tuple_routing() and apply_handle_tuple_routing()->apply_handle_delete_internal(), which requires the index or sequential scan, right? LGTM. > Apart from above, I found there are quite a few duplicate codes related to partition > handling(e.g. apply_handle_tuple_routing), so I tried to extract some > common logic to simplify the codes. Please see 0002 for this refactoring. IIUC, you wanted to remove the application code from apply_handle_tuple_routing() and put only a part partition detection. Is it right? Anyway, here are comments. 01. apply_handle_insert() ``` + targetRelInfo = edata->targetRelInfo; + /* For a partitioned table, insert the tuple into a partition. */ if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - apply_handle_tuple_routing(edata, - remoteslot, NULL, CMD_INSERT); - else - apply_handle_insert_internal(edata, edata->targetRelInfo, - remoteslot); + remoteslot = apply_handle_tuple_routing(edata, CMD_INSERT, remoteslot, + &targetRelInfo); + + /* For a partitioned table, insert the tuple into a partition. */ + apply_handle_insert_internal(edata, targetRelInfo, remoteslot); ``` This part contains same comments, and no need to subsctitute in case of normal tables. How about: ``` - /* For a partitioned table, insert the tuple into a partition. */ + /* + * Find the actual target table if the table is partitioned. Otherwise, use + * the same table as the remote one. + */ if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - apply_handle_tuple_routing(edata, - remoteslot, NULL, CMD_INSERT); + remoteslot = apply_handle_tuple_routing(edata, CMD_INSERT, remoteslot, + &targetRelInfo); else - apply_handle_insert_internal(edata, edata->targetRelInfo, - remoteslot); + targetRelInfo = edata->targetRelInfo; + + /* Insert a tuple to the target table */ + apply_handle_insert_internal(edata, targetRelInfo, remoteslot); ``` 02. apply_handle_tuple_routing() ``` /* - * This handles insert, update, delete on a partitioned table. + * Determine the partition in which the tuple in slot is to be inserted, and ... ``` But this function is called from delete_internal(). How about "Determine the partition to which the tuple in the slot belongs."? 03. apply_handle_tuple_routing() Do you have a reason why this does not return `ResultRelInfo *` but `TupleTableSlot *`? Not sure, but it is more proper for me to return the table info because this is a routing function. 04. apply_handle_update() ``` + targetRelInfo = edata->targetRelInfo; + targetrel = rel; + remoteslot_root = remoteslot; ``` Here I can say the same thing as 1. 05. apply_handle_update_internal() It looks the ordering of function's implementations are changed. Is it intentaional? before apply_handle_update apply_handle_update_internal apply_handle_delete apply_handle_delete_internal FindReplTupleInLocalRel apply_handle_tuple_routing after apply_handle_update apply_handle_delete apply_handle_delete_internal FindReplTupleInLocalRel apply_handle_tuple_routing apply_handle_update_internal 06. apply_handle_delete_internal() ``` + targetRelInfo = edata->targetRelInfo; + targetrel = rel; + ``` Here I can say the same thing as 1. Best regards, Hayato Kuroda FUJITSU LIMITED
RE: Remove duplicate table scan in logical apply worker and code refactoring
From
"Zhijie Hou (Fujitsu)"
Date:
On Wednesday, July 31, 2024 5:07 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote: > > Dear Hou, > > > When reviewing the code in logical/worker.c, I noticed that when > > applying a cross-partition update action, it scans the old partition twice. > > I am attaching the patch 0001 to remove this duplicate table scan. > > Just to clarify, you meant that FindReplTupleInLocalRel() are called in > apply_handle_tuple_routing() and > apply_handle_tuple_routing()->apply_handle_delete_internal(), > which requires the index or sequential scan, right? LGTM. Thanks for reviewing the patch, and your understanding is correct. Here is the updated patch 0001. I removed the comments as suggested by Amit. Since 0002 patch is only refactoring the code and I need some time to review the comments for it, I will hold it until the 0001 is committed. Best Regards, Hou zj
Attachment
RE: Remove duplicate table scan in logical apply worker and code refactoring
From
"Hayato Kuroda (Fujitsu)"
Date:
Dear Hou, > Thanks for reviewing the patch, and your understanding is correct. > > Here is the updated patch 0001. I removed the comments as suggested by Amit. > > Since 0002 patch is only refactoring the code and I need some time to review > the comments for it, I will hold it until the 0001 is committed. Thanks for updating the patch. I did a performance testing with v2-0001. Before: 15.553 [s] After: 7.593 [s] I used the attached script for setting up. I used almost the same setting and synchronous replication is used. [machine] CPU(s): 120 Model name: Intel(R) Xeon(R) CPU E7-4890 v2 @ 2.80GHz Core(s) per socket: 15 Socket(s): 4 Best regards, Hayato Kuroda FUJITSU LIMITED
Attachment
On Thu, Aug 1, 2024 at 7:29 AM Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> wrote: > > > Thanks for reviewing the patch, and your understanding is correct. > > > > Here is the updated patch 0001. I removed the comments as suggested by Amit. > > > > Since 0002 patch is only refactoring the code and I need some time to review > > the comments for it, I will hold it until the 0001 is committed. > > Thanks for updating the patch. I did a performance testing with v2-0001. > > Before: 15.553 [s] > After: 7.593 [s] > Thanks for the testing. I have pushed the patch. -- With Regards, Amit Kapila.