Re: [HACKERS] increasing the default WAL segment size

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