Thread: Remove duplicate table scan in logical apply worker and code refactoring

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

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.

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.

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

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

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

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.