I rebased this patch; it's failing to apply due to minor concurrent
changes in PostgresNode.pm. I squashed the patches in a series that
made the most sense to me.
I have a question about static variable lastFoundOldestSeg in
FindOldestXLogFileSegNo. It may be set the first time the function
runs; if it is, the function never again does anything, it just returns
that value. In other words, the static value is never reset; it never
advances either. Isn't that strange? I think the coding is to assume
that XLogCtl->lastRemovedSegNo will always be set, so its code will
almost never run ... except when the very first wal file has not been
removed yet. This seems weird and pointless. Maybe we should think
about this differently -- example: if XLogGetLastRemovedSegno returns
zero, then the oldest file is the zeroth one. In what cases this is
wrong? Maybe we should fix those.
Regarding the PostgresNode change in 0001, I think adding a special
parameter for primary_slot_name is limited. I'd like to change the
definition so that anything that you give as a parameter that's not one
of the recognized keywords (has_streaming, etc) is tested to see if it's
a GUC; and if it is, then put it in postgresql.conf. This would have to
apply both to PostgresNode::init() as well as
PostgresNode::init_from_backup(), obviously, since it would make no
sense for the APIs to diverge on this point. So you'd be able to do
$node->init_from_backup(allow_streaming => 1, work_mem => "4MB");
without having to add code to init_from_backup to handle work_mem
specifically. This could be done by having a Perl hash with all the GUC
names, that we could read lazily from "postmaster --describe-config" the
first time we see an unrecognized keyword as an option to init() /
init_from_backup().
I edited the doc changes a bit.
I don't know what to think of 0003 yet. Has this been agreed to be a
good idea?
I also made a few small edits to the code; all cosmetic so far:
* added long_desc to the new GUC; it now reads:
{"max_slot_wal_keep_size", PGC_SIGHUP, REPLICATION_SENDING,
gettext_noop("Sets the maximum size of WAL space reserved by replication slots."),
gettext_noop("Replication slots will be marked as failed, and segments released "
"for deletion or recycling, if this much space is occupied by WAL "
"on disk."),
* updated the comment to ConvertToXSegs() which is now being used for
this purpose
* remove outdated comment to GetWalAvailability; it was talking about
restBytes parameter that no longer exists
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
