Re: Separate HEAP WAL replay logic into its own file - Mailing list pgsql-hackers

Hi,

I'm reviewing patches in commitfest 2024-07:
https://commitfest.postgresql.org/48/

This is the 5th patch:
https://commitfest.postgresql.org/48/5054/

FYI: https://commitfest.postgresql.org/48/4681/ is my patch.

In <DAB53475-2470-4431-A3D1-A2A855530EAB@ebay.com>
  "Re: Separate HEAP WAL replay logic into its own file" on Tue, 18 Jun 2024 01:12:42 +0000,
  "Li, Yong" <yoli@ebay.com> wrote:

> As a newcomer, when I was walking through the code looking
> for WAL replay related code, it was relatively easy for me
> to find them for the B-Tree access method because of the
> “xlog” hint in the file names. It took me a while to
> find the same for the heap access method. When I finally
> found them (via text search), it was a small
> surprise. Having different file organizations for
> different access methods gives me this urge to make
> everything consistent. I think it will make it easier for
> newcomers, and it will reduce the mental load for everyone
> to remember that heap replay is inside the heapam.c not
> some “???xlog.c”.

It makes sense.


Here are my comments for your patch:

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".

2. I confirmed that all heapam.c -> heapam_xlog.c/heapam.h
   moves don't change implementations. I re-moved moved
   codes to heapam.c and there is no diff in heapam.c.

3. Could you remove '#include "access/heapam_xlog.h"' from
   heapam.c because it's needless now.

   BTW, it seems that we can remove more includes from
   heapam.c:

----
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index bc6d4868975..f1671072576 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -31,42 +31,24 @@
  */
 #include "postgres.h"
 
-#include "access/bufmask.h"
 #include "access/heapam.h"
-#include "access/heapam_xlog.h"
 #include "access/heaptoast.h"
 #include "access/hio.h"
 #include "access/multixact.h"
-#include "access/parallel.h"
-#include "access/relscan.h"
 #include "access/subtrans.h"
 #include "access/syncscan.h"
-#include "access/sysattr.h"
-#include "access/tableam.h"
-#include "access/transam.h"
 #include "access/valid.h"
 #include "access/visibilitymap.h"
-#include "access/xact.h"
-#include "access/xlog.h"
 #include "access/xloginsert.h"
-#include "access/xlogutils.h"
-#include "catalog/catalog.h"
 #include "commands/vacuum.h"
-#include "miscadmin.h"
 #include "pgstat.h"
-#include "port/atomics.h"
 #include "port/pg_bitutils.h"
-#include "storage/bufmgr.h"
-#include "storage/freespace.h"
 #include "storage/lmgr.h"
 #include "storage/predicate.h"
 #include "storage/procarray.h"
-#include "storage/standby.h"
 #include "utils/datum.h"
 #include "utils/injection_point.h"
 #include "utils/inval.h"
-#include "utils/relcache.h"
-#include "utils/snapmgr.h"
 #include "utils/spccache.h"
---

   We may want to work on removing needless includes as a
   separated cleanup task.

4. Could you remove needless includes from heapam_xlog.c? It
   seems that we can remove the following includes:

----
diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c
index b372f2b4afc..af4976f382d 100644
--- a/src/backend/access/heap/heapam_xlog.c
+++ b/src/backend/access/heap/heapam_xlog.c
@@ -16,16 +16,11 @@
 
 #include "access/bufmask.h"
 #include "access/heapam.h"
-#include "access/heapam_xlog.h"
-#include "access/transam.h"
 #include "access/visibilitymap.h"
 #include "access/xlog.h"
 #include "access/xlogutils.h"
-#include "port/atomics.h"
-#include "storage/bufmgr.h"
 #include "storage/freespace.h"
 #include "storage/standby.h"
-#include "utils/relcache.h"
----

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.


Thanks,
-- 
kou