RE: [bug fix] Cascaded standby cannot start after a clean shutdown

From: "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>
To: 'Michael Paquier' <michael(at)paquier(dot)xyz>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: [bug fix] Cascaded standby cannot start after a clean shutdown
Date: 2018-02-19 03:01:15
Message-ID: 0A3221C70F24FB45833433255569204D1F8C977C@G01JPEXMBYT05
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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;
else
readLen = receivedUpto % XLogSegSize - targetPageOff;
}
else
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.

Regards
Takayuki Tsunakawa

Attachment Content-Type Size
zerofill_walblock_on_standby_v2.patch application/octet-stream 1.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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