|From:||"Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>|
|To:||'Michael Paquier' <michael(at)paquier(dot)xyz>|
|Subject:||RE: [bug fix] Cascaded standby cannot start after a clean shutdown|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Thank you for reviewing.
From: Michael Paquier [mailto:michael(at)paquier(dot)xyz]
> It seems to me that the consolidation of the page read should happen directly
> in xlogreader.c and not even in one of its callbacks so as no garbage data
> is presented back to the caller using its own XLogReader.
> I think that you need to initialize XLogReaderState->readBuf directly in
> ReadPageInternal() before reading a page and you should be good. With your
> patch you get visibly only one portion of things addressed, what of other
> code paths using xlogreader.c's APIs like pg_rewind, 2PC code and such?
ReadPageInternal() doesn't know where the end of valid WAL is, so it cannot determine where to do memset(0). For example, in non-streaming cases, it reads the entire WAL block into readbuf, including the garbage.
* If the current segment is being streamed from master, calculate how
* much of the current page we have received already. We know the
* requested record has been received, but this is for the benefit of
* future calls, to allow quick exit at the top of this function.
if (readSource == XLOG_FROM_STREAM)
if (((targetPagePtr) / XLOG_BLCKSZ) != (receivedUpto / XLOG_BLCKSZ))
readLen = XLOG_BLCKSZ;
readLen = receivedUpto % XLogSegSize - targetPageOff;
readLen = XLOG_BLCKSZ;
So we have to zero-fill the empty space of a WAL block before writing it. Currently, the master does that in AdvanceXLInsertBuffer(), StartupXLOG(), XLogFileCopy(). The cascading standby receives WAL from the master block by block, so it doesn't suffer from the garbage. pg_receivexlog zero-fills a new WAL file.
> Please note that your patch has some unnecessary garbage in two places:
Ouch, cleaned up.
> + if (zbuffer == NULL)
> + zbuffer = palloc0(XLOG_BLCKSZ);
> You could just use a static buffer which is MemSet'd with 0 only if necessary.
I wanted to allocate memory only when necessary. Currently, only the cascaded standby needs this. To that end, I moved the memory allocation code to a right place. Thanks for letting me notice this.
|Next Message||Craig Ringer||2018-02-19 03:21:49||Re: Documenting PROVE_TESTS in section of TAP tests|
|Previous Message||David Rowley||2018-02-19 02:48:27||Re: ALTER TABLE ADD COLUMN fast default|