On Mon, Nov 8, 2021 at 8:27 AM Robert Haas <robertmh...@gmail.com> wrote: > Perhaps. That's related to the point I made before, that it might be a > good idea to be more clear about which of these functions are intended > to be used in recovery and which ones are intended to be used in > normal running. I don't rule out the possibility of doing some more > research here and tightening that up, but I don't want to go start > tweaking things unless I'm sure I know what I'm talking about. This > stuff is so confusing that I don't want to take any risk of making > changes that turn out to be incorrect.
Well I went through and it seems to be OK: all the existing callers of that function first verify that we're not in recovery. The patch to make logical decoding work in standby mode might change that, but as of now it's so. So in the attached patch, I have added the assertion and comments that you have proposed there. I also removed the misleading comment we discussed before. In addition to that, I renamed the local variable ThisTimeLineID to replayTLI, and I also renamed XLogCtl->ThisTimeLineID to InsertTimeLineID, which I think should make things clearer: imagine if things that were used for different purposes had different names! Even with this patch, the name ThisTimeLineID is still used for multiple purposes. It remains part of the CheckPoint struct, and also part of the xl_end_of_recovery struct. But in both of those cases, the name ThisTimeLineID actually makes sense, because what we're thinking about is whether there's a timeline change, so we have ThisTimeLineID and PrevTimeLineID and it seems fairly clear what that's supposed to mean. Thoughts? -- Robert Haas EDB: http://www.enterprisedb.com
0001-More-cleanup-of-ThisTimeLineID.patch
Description: Binary data
