Re: Remove page-read callback from XLogReaderState.

Hi,

I applied your v15 patchset to master
ed2c7f65bd9f15f8f7cd21ad61602f983b1e72e9. Here are three feedback points
for you:

= 1. Build error when WAL_DEBUG is defined manually =
How to reproduce:

$ sed -i -E -e 's|^/\* #define WAL_DEBUG \*/$|#define WAL_DEBUG|'
src/include/pg_config_manual.h
$ ./configure && make

Expected: PostgreSQL is successfully made.
Actual: I got the following make error:

>>>>>>>>
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -O2
-I../../../../src/include -D_GNU_SOURCE -c -o xlog.o xlog.c
In file included from /usr/include/x86_64-linux-gnu/bits/types/stack_t.h:23,
from /usr/include/signal.h:303,
from ../../../../src/include/storage/sinval.h:17,
from ../../../../src/include/access/xact.h:22,
from ../../../../src/include/access/twophase.h:17,
from xlog.c:33:
xlog.c: In function ‘XLogInsertRecord’:
xlog.c:1219:56: error: called object is not a function or function pointer
1219 | debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
| ^~~~
xlog.c:1219:19: error: too few arguments to function ‘XLogReaderAllocate’
1219 | debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
| ^~~~~~~~~~~~~~~~~~
In file included from ../../../../src/include/access/clog.h:14,
from xlog.c:25:
../../../../src/include/access/xlogreader.h:243:25: note: declared here
243 | extern XLogReaderState *XLogReaderAllocate(int wal_segment_size,
| ^~~~~~~~~~~~~~~~~~
make[4]: *** [<builtin>: xlog.o] Error 1
<<<<<<<<

The following chunk in 0002 seems to be the cause of the error. There is
no comma between two NULLs.

>>>>>>>>
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index e570e56a24..f9b0108602 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
(..snipped..)
@@ -1225,8 +1218,7 @@ XLogInsertRecord(XLogRecData *rdata,
appendBinaryStringInfo(&recordBuf, rdata->data, rdata->len);

if (!debug_reader)
- debug_reader = XLogReaderAllocate(wal_segment_size, NULL,
- XL_ROUTINE(), NULL);
+ debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);

if (!debug_reader)
{
<<<<<<<<

= 2. readBuf allocation in XLogReaderAllocate =
AFAIU, not XLogReaderAllocate() itself but its caller is now responsible
for allocating XLogReaderState->readBuf. However, the following code still
remains in src/backend/access/transam/xlogreader.c:

>>>>>>>>
74 XLogReaderState *
75 XLogReaderAllocate(int wal_segment_size, const char *waldir,
76 WALSegmentCleanupCB cleanup_cb)
77 {
:
98 state->readBuf = (char *) palloc_extended(XLOG_BLCKSZ,
99 MCXT_ALLOC_NO_OOM);
<<<<<<<<

Is this okay?

= 3. XLOG_FROM_ANY assigned to global readSource =
Regarding the following chunk in 0003:

>>>>>>>>
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 6b42d9015f..bcb4ef270f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -804,18 +804,14 @@ static XLogSegNo openLogSegNo = 0;
* These variables are used similarly to the ones above, but for reading
* the XLOG. Note, however, that readOff generally represents the offset
* of the page just read, not the seek position of the FD itself, which
- * will be just past that page. readLen indicates how much of the current
- * page has been read into readBuf, and readSource indicates where we got
- * the currently open file from.
+ * will be just past that page. readSource indicates where we got the
+ * currently open file from.
* Note: we could use Reserve/ReleaseExternalFD to track consumption of
* this FD too; but it doesn't currently seem worthwhile, since the XLOG is
* not read by general-purpose sessions.
*/
static int readFile = -1;
-static XLogSegNo readSegNo = 0;
-static uint32 readOff = 0;
-static uint32 readLen = 0;
-static XLogSource readSource = XLOG_FROM_ANY;
+static XLogSource readSource = 0; /* XLOG_FROM_* code */

/*
* Keeps track of which source we're currently reading from. This is
<<<<<<<<

I think it is better to keep the line "static XLogSource readSource =
XLOG_FROM_ANY;". XLOG_FROM_ANY is already defined as 0 in
src/backend/access/transam/xlog.c.

Regards,
Takashi

2020年7月2日(木) 13:53 Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>:

> cfbot is complaining as this is no longer applicable. Rebased.
>
> In v14, some reference to XLogReaderState parameter to read_pages
> functions are accidentally replaced by the reference to the global
> variable xlogreader. Fixed it, too.
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

--
Takashi Menjo <takashi(dot)menjo(at)gmail(dot)com>