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