Date: Tue, 31 Mar 2020 12:07:55 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: jgdr@dalibo.com, andres@anarazel.de, michael@paquier.xyz,
	sawada.mshk@gmail.com, peter.eisentraut@2ndquadrant.com,
	pgsql-hackers@lists.postgresql.org, thomas.munro@enterprisedb.com,
	sk@zsrv.org, michael.paquier@gmail.com
Subject: Re: [HACKERS] Restricting maximum keep segments by repslots
Message-ID: <20200331150755.GA3858@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200331.142016.971756479150607227.horikyota.ntt@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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

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