Re: [HACKERS] increasing the default WAL segment size - Mailing list pgsql-hackers
From | Beena Emerson |
---|---|
Subject | Re: [HACKERS] increasing the default WAL segment size |
Date | |
Msg-id | CAOG9ApHUj10U6ryyTBMqc4pu_yoghULF1YCBwefp4g+MovSJQA@mail.gmail.com Whole thread Raw |
In response to | Re: [HACKERS] increasing the default WAL segment size (Andres Freund <andres@anarazel.de>) |
Responses |
Re: [HACKERS] increasing the default WAL segment size
|
List | pgsql-hackers |
On Wed, Sep 13, 2017 at 2:58 PM, Andres Freund <andres@anarazel.de> wrote: > Hi, > > On 2017-09-06 20:24:16 +0530, Beena Emerson wrote: >> > - pg_standby's RetrieveWALSegSize() does too much for it's name. It >> > seems quite weird that a function named that way has the section below >> > "/* check if clean up is necessary */" >> >> we set 2 cleanup related variables once WalSegSize is set, namely >> need_cleanup and exclusiveCleanupFileName. Does >> SetWALSegSizeAndCleanupValues look good? > > It's better, but see below. > > >> > - the way you redid the ReadControlFile() invocation doesn't quite seem >> > right. Consider what happens if XLOGbuffers isn't -1 - then we >> > wouldn't read the control file, but you unconditionally copy it in >> > XLOGShmemInit(). I think we instead should introduce something like >> > XLOGPreShmemInit() that reads the control file unless in bootstrap >> > mode. Then get rid of the second ReadControlFile() already present. >> >> I did not think it was necessary to create a new function, I have >> simply added the check and >> function call within the XLOGShmemInit(). > > Which is wrong. XLogShmemSize() already needs to know the actual size, > otherwise we allocate the wrong shmem size. You may sometimes succeed > nevertheless because we leave some slop unused shared memory space, but > it's not ok to rely on. See the refactoring I did in 0001. > > Changes: > - refactored the way the control file was handled, moved it to separate > phase. I wrote this last and it's late, so I'm not yet fully confident > in it, but it survives plain and EXEC_BACKEND builds. This also gets > rid of ferrying wal_segment_size through the EXEC_BACKEND variable > stuff, which didn't really do much, given how many other parts weren't > carried over. > - renamed all the non-postgres binary version of wal_segment_size to > WalSegSz, diverging seems pointless, and the WalSegsz seems > inconsistent. > - changed malloc in pg_waldump's search_directory() to a stack > allocation. Less because of efficiency, more because there wasn't any > error handling. > - removed redundant char * -> XLogPageHeader -> XLogLongPageHeader casting. > - replace new malloc with pg_malloc in initdb (failure handling!) > - replaced the floating point logic in pretty_wal_size with a, imo much > simpler, (sz % 1024) == 0 > - it's inconsistent that the new code for pg_standby was added to the > top of the file, where all the customizable stuff resides. > - other small changes > > Issues: > > - I think the pg_standby stuff isn't correct. And it's hard to > understand. Consider the case where the first file restored is *not* a > timeline history file, but *also* not a complete file. We'll start to > spew "not enough data in file" errors and such, which we previously > didn't. My preferred solution would be to remove pg_standby ([1]), > but that's probably not quick enough. Unless we can quickly agree on > that, I think we need to refactor this a bit, I've done so in the > attached, but it's untested. Could you please verify it works and if > not fix it up? > > What do you think? The change looks good and is working as expected. PFA the updated patch after running pgindent. Thank you, Beena Emerson EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
pgsql-hackers by date: