v4-0002-review.patch

From 17aed41dfdd452727643a40e5dba6c0c4705c64a Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@vondra.me>
Date: Tue, 17 Sep 2024 16:37:49 +0200
Subject: [PATCH v4 02/11] review

---
 .../replication/logical/reorderbuffer.c        | 10 ++++++++--
 .../logical/reorderbuffer_compression.c        | 18 +++++++++++++++++-
 src/include/replication/reorderbuffer.h        |  2 ++
 .../replication/reorderbuffer_compression.h    |  8 ++++++++
 4 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index c36179d44b5..f5c51316d9d 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -113,8 +113,6 @@
 #include "utils/rel.h"
 #include "utils/relfilenumbermap.h"
 
-int			logical_decoding_spill_compression = REORDER_BUFFER_NO_COMPRESSION;
-
 /* entry for a hash table we use to map from xid to our transaction state */
 typedef struct ReorderBufferTXNByIdEnt
 {
@@ -176,6 +174,11 @@ typedef struct ReorderBufferToastEnt
 									 * main tup */
 } ReorderBufferToastEnt;
 
+/*
+ * XXX seems wrong to move ReorderBufferDiskChange typedef, it should be private
+ * in this file, even with compression IMHO
+ */
+
 #define IsSpecInsert(action) \
 ( \
 	((action) == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT) \
@@ -210,6 +213,9 @@ static const Size max_changes_in_memory = 4096; /* XXX for restore only */
 /* GUC variable */
 int			debug_logical_replication_streaming = DEBUG_LOGICAL_REP_STREAMING_BUFFERED;
 
+/* Compression strategy for spilled data. */
+int			logical_decoding_spill_compression = REORDER_BUFFER_NO_COMPRESSION;
+
 /* ---------------------------------------
  * primary reorderbuffer support routines
  * ---------------------------------------
diff --git a/src/backend/replication/logical/reorderbuffer_compression.c b/src/backend/replication/logical/reorderbuffer_compression.c
index 77f5c76929b..b4fea15ebd9 100644
--- a/src/backend/replication/logical/reorderbuffer_compression.c
+++ b/src/backend/replication/logical/reorderbuffer_compression.c
@@ -9,6 +9,8 @@
  * IDENTIFICATION
  *	  src/backend/access/common/reorderbuffer_compression.c
  *
+ *
+ * XXX I'd rename this to reorderbuffer_lz4 or something like that.
  *-------------------------------------------------------------------------
  */
 #include "postgres.h"
@@ -301,6 +303,8 @@ lz4_DecompressData(char *src, Size src_size, char **dst, Size dst_size)
 #endif
 }
 
+/* XXX The rest should be in reorderbuffer.c, with the rest of ReorderBuffer functions */
+
 /*
  * Allocate a new Compressor State, depending on the compression method.
  */
@@ -376,6 +380,12 @@ ReorderBufferCompress(ReorderBuffer *rb, ReorderBufferDiskHeader **header,
 		/* LZ4 Compression */
 		case REORDER_BUFFER_LZ4_COMPRESSION:
 		{
+			/*
+			 * XXX Won't this cause a lot of palloc/pfree traffic? We allocate a new
+			 * buffer for every compression, only to immediately throw it away. Look
+			 * at what astreamer_lz4_compressor_content does to reuse a buffer, maybe
+			 * we should do that here too?
+			 */
 			char	   *dst = NULL;
 			Size		dst_size = 0;
 			char	   *src = (char *) rb->outbuf + sizeof(ReorderBufferDiskHeader);
@@ -428,6 +438,12 @@ ReorderBufferCompress(ReorderBuffer *rb, ReorderBufferDiskHeader **header,
 
 /*
  * Decompress data read from disk and copy it into the ReorderBuffer.
+ *
+ * XXX Does it make sense to support both "regular" and "streaming" for lz4?
+ * Isn't one of those clearly better for this use case?
+ *
+ * XXX Also, we only ever create LZ4StreamingCompressorState, so does that
+ * work for non-streaming state?
  */
 void
 ReorderBufferDecompress(ReorderBuffer *rb, char *data,
@@ -479,7 +495,7 @@ ReorderBufferDecompress(ReorderBuffer *rb, char *data,
 		/* LZ4 streaming decompression */
 		case REORDER_BUFFER_STRAT_LZ4_STREAMING:
 			{
-				char	   *buf;
+				char	   *buf;	/* XXX shouldn't this be set to NULL explicitly? */
 				Size		src_size = header->size - sizeof(ReorderBufferDiskHeader);
 				Size		buf_size = header->raw_size;
 
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index f1562a77719..1a2ccda2eca 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -9,6 +9,7 @@
 #ifndef REORDERBUFFER_H
 #define REORDERBUFFER_H
 
+/* XXX we don't need any lz4 stuff in this header, right? */
 #ifdef USE_LZ4
 #include <lz4.h>
 #endif
@@ -30,6 +31,7 @@
 /* GUC variables */
 extern PGDLLIMPORT int logical_decoding_work_mem;
 extern PGDLLIMPORT int debug_logical_replication_streaming;
+extern PGDLLIMPORT int logical_decoding_spill_compression;
 
 /* possible values for debug_logical_replication_streaming */
 typedef enum
diff --git a/src/include/replication/reorderbuffer_compression.h b/src/include/replication/reorderbuffer_compression.h
index 9aa8aea56f4..be739d9ebeb 100644
--- a/src/include/replication/reorderbuffer_compression.h
+++ b/src/include/replication/reorderbuffer_compression.h
@@ -37,6 +37,14 @@ typedef enum ReorderBufferCompressionStrategy
 } ReorderBufferCompressionStrategy;
 
 /* Disk serialization support datastructures */
+
+/* XXX but what does ReorderBufferDiskHeader represent? what is it for?
+ * XXX Also, shouldn't it be ReorderBufferDiskChangeHeader? It's a header
+ * for a change, not for a disk.
+ * XXX I don't quite understand why we renamed this, DiskChange seems quite
+ * fine to me, even if it gets new fields for compression. Still, I'd keep
+ * it as private in reorderbuffer.c.
+ */
 typedef struct ReorderBufferDiskHeader
 {
 	ReorderBufferCompressionStrategy comp_strat; /* Compression strategy */
-- 
2.46.0