Re: Remove page-read callback from XLogReaderState.

Thank you for the comment, Menjo-san, and noticing me of that, Michael.

Sorry for late reply.

At Fri, 17 Jul 2020 14:14:44 +0900, Takashi Menjo <takashi(dot)menjo(at)gmail(dot)com> wrote in
> 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>