Re: [HACKERS] increasing the default WAL segment size

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?

Regards,

Andres

[1] http://archives.postgresql.org/message-id/20170913064824.rqflkadxwpboabgw%40alap3.anarazel.de

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers