Re: Remove page-read callback from XLogReaderState.

Thank you for the comment and 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 =
..
> Expected: PostgreSQL is successfully made.
> Actual: I got the following make error:
...
> 1219 | debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);

Ah, I completely forgot about WAL_DEBUG paths. Fixed.

> = 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?

Oops! That's silly. However, I put a rethink on this. The reason of
the moving of responsibility comes from the fact that the actual
subject that fills-in the buffer is the callers of xlogreader, who
knows its size. In that light it's quite strange that xlogreader
works based on the fixed size of XLOG_BLCKSZ. I don't think it is
useful just now, but I changed 0004 so that XLOG_BLCKSZ is eliminated
from xlogreader.c. Buffer allocation is restored to
XLogReaderAllocate.

(But, I'm not sure it's worth doing..)

> = 3. XLOG_FROM_ANY assigned to global readSource =
> Regarding the following chunk in 0003:
...
> -static XLogSource readSource = XLOG_FROM_ANY;
> +static XLogSource readSource = 0; /* XLOG_FROM_* code */
>
> 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.

That seems to be a mistake while past rebasding. XLOG_FROM_ANY is the
right thing to use here.

The attached is the rebased, and fixed version.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center