Re: [HACKERS] Restricting maximum keep segments by repslots

On 2020-Mar-31, Kyotaro Horiguchi wrote:

> Thank you for looking this and trouble rebasing!
>
> At Mon, 30 Mar 2020 20:03:27 -0300, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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.

I'm not sure if I explained my proposal clearly. What if
XLogGetLastRemovedSegno returning zero means that every segment is
valid? We don't need to scan pg_xlog at all.

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

Hmm. I like this idea in general, but I'm not sure I want to introduce
it in this form right away. For the time being I realized while waking
up this morning we can just use $node->append_conf() in the
018_replslot_limit.pl file, like every other place that needs a special
config. There's no need to change the test infrastructure for this.

I'll go through this again. Many thanks,

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services