RE: Remove duplicate table scan in logical apply worker and code refactoring - Mailing list pgsql-hackers

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