Re: [HACKERS] Restricting maximum keep segments by repslots

Thank you for looking this and trouble rebasing!

At Mon, 30 Mar 2020 20:03:27 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in 
> 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.

That's right, but without the static variable, every call to the
pg_replication_slots view before the fist checkpoint causes scanning
pg_xlog. XLogCtl->lastRemovedSegNo advances only at a checkpoint, so
it is actually right that the return value from
FindOldestXLogFileSegNo doesn't change until the first checkpoint.

Also we could set XLogCtl->lastRemovedSegNo at startup, but the
scanning on pg_xlog is useless in most cases.

I avoided to update the XLogCtl->lastRemovedSegNo directlry, but the
third way would be if XLogGetLastRemovedSegno() returned 0, then set
XLogCtl->lastRemovedSegNo by scanning the WAL directory. The attached
takes this way.

> 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().

Done that way. We could exclude "known" parameters by explicitly
delete the key at reading it, but I choosed to enumerate the known
keywords.  Although it can be used widely but actually I changed only
018_repslot_limit.pl to use the feature.

> 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?

So it was a separate patch. I think it has not been approved nor
rejected.  The main objective of the patch is preventing
pg_replication_slots.wal_status from strange coming back from the
"lost" state to other states. However, in the first place I doubt that
it's right that logical replication sends the content of a WAL segment
already recycled.

> 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

Thank you for the fixes. All of the looks fine.

I fixed several typos. (s/requred/required/, s/devinitly/definitely/,
s/errror/error/)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center