On Wed, Sep 15, 2021 at 10:32 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Putting these changes into 0001 seems to make no sense. It seems like
> they should be part of 0003, or maybe a new 0004 patch.
After looking at this a little bit more, I think it's really necessary
to separate out all of your changes into separate patches at least for
initial review. It's particularly important to separate code movement
changes from other kinds of changes. 0001 was just moving code before,
and so was 0002, but now both are making other changes, which is not
easy to see from looking at the 'git diff' output. For that reason
it's not so easy to understand exactly what you've changed here and
analyze it.
I poked around a little bit at these patches, looking for
perhaps-interesting global variables upon which the code called from
XLogAcceptWrites() would depend with your patches applied. The most
interesting ones seem to be (1) ThisTimeLineID, which you mentioned
and which may be fine but I'm not totally convinced yet, (2)
LocalXLogInsertAllowed, which is probably not broken but I'm thinking
we may want to redesign that mechanism somehow to make it cleaner, and
(3) CheckpointStats, which is called from RemoveXlogFile which is
called from RemoveNonParentXlogFiles which is called from
CleanupAfterArchiveRecovery which is called from XLogAcceptWrites.
This last one is actually pretty weird already in the existing code.
It sort of looks like RemoveXlogFile() only expects to be called from
the checkpointer (or a standalone backend) so that it can update
CheckpointStats and have that just work, but actually it's also called
from the startup process when a timeline switch happens. I don't know
whether the fact that the increments to ckpt_segs_recycled get lost in
that case should be considered an intentional behavior that should be
preserved or an inadvertent mistake.
So I think you've covered most of the necessary things here, with
probably some more discussion needed on whether you've done the right
things...
--
Robert Haas
EDB: http://www.enterprisedb.com
