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

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