Re: Separate HEAP WAL replay logic into its own file

Hi,

In <599E67D2-2929-4858-B8BC-F9C4AE889BF6(at)ebay(dot)com>
"Re: Separate HEAP WAL replay logic into its own file" on Fri, 26 Jul 2024 07:56:12 +0000,
"Li, Yong" <yoli(at)ebay(dot)com> wrote:

>> 1. Could you create your patch by "git format-patch -vN master"
>> or something? If you create your patch by "git format-patch",
>> we can apply your patch by "git am XXX.patch".
>>
>
> Thanks for your review. I’ve updated the patch to follow your
> suggested format.

Thanks. I could apply your patch by "git am
v2-0001-heapam_refactor.patch".

Could you use the following format for the commit message
next time?

----
${TITLE}

${DESCRIPTION}
----

For example:

----
Separate HEAP WAL replay logic into its own file

Most access methods (i.e. nbtree and hash) use a separate
file with "xlog" in its name for its WAL replay logic. Heap
is one exception of this convention. To make it easier for
newcomers to find the WAL replay logic for the heap access
method, this patch isolates heap's replay logic in a new
heapam_xlog.c file. This patch is a pure refactoring with no
change to the logic.
----

This is a commonly used Git's commit message format. See
also other commit messages by "git log".

>> 5. There are still WAL related codes in heapam.c:
>>
>> 4.1. log_heap_update()
>> 4.2. log_heap_new_cid()
>> 4.3. if (RelationNeedsWAL()) {...} in heap_insert()
>> 4.4. if (needwal) {...} in heap_multi_insert()
>> 4.5. if (RelationNeedsWAL()) {...} in heap_delete()
>> 4.6. if (RelationNeedsWAL()) {...}s in heap_update()
>> 4.7. if (RelationNeedsWAL()) {...} in heap_lock_tuple()
>> 4.8. if (RelationNeedsWAL()) {...} in heap_lock_updated_tuple_rec()
>> 4.9. if (RelationNeedsWAL()) {...} in heap_finish_speculative()
>> 4.10. if (RelationNeedsWAL()) {...} in heap_abort_speculative()
>> 4.11. if (RelationNeedsWAL()) {...} in heap_inplace_update()
>> 4.12. log_heap_visible()
>>
>> Should we move them to head_xlog.c too?
>>
>> If we should do it, separated commits will be easy to
>> review. For example, the 0001 patch moves existing codes
>> to head_xlog.c as-is. The 0002 patch extracts WAL related
>> codes in heap_insert() to heap_xlog.c and so on.
>
> I followed the convention of most access methods. The “xlog”
> file includes the WAL replay logic only. The logic that generates
> the log records themselves stays with the code that performs
> the changes. Take nbtree as an example, you can also find
> WAL generating code in several _bt_insertxxx() functions inside
> the nbtinsert.c file.

You're right. Sorry.

I think that this proposal is reasonable but we need to get
attention from a committer to move forward this proposal.

Thanks,
--
kou