Re: Remove page-read callback from XLogReaderState. - Mailing list pgsql-hackers

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.menjo@gmail.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.ntt@gmail.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.menjo@gmail.com>