On Wed, Sep 15, 2021 at 10:32 AM Robert Haas <robertmh...@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
