On Wed, Nov 24, 2021 at 12:16 PM Heikki Linnakangas <hlinn...@iki.fi> wrote: > And here's another rebase, now that Robert got rid of ReadRecPtr and > EndRecPtr.
In general, I think 0001 is a good idea, but the comment that says "Set the XLP_FIRST_IS_OVERWRITE_CONTRECORD flag on the page header" seems to me to be telling the reader about what's already obvious instead of explaining to them the thing they might have missed. GetXLogBuffer() says that it's only safe to use if you hold a WAL insertion lock and don't go backwards, and here you don't hold a WAL insertion lock and I guess you're not going backwards only because you're staying in exactly the same place? It seems to me that the only reason this is safe is because, at the time this is called, only the startup process is able to write WAL, and therefore the race condition that would otherwise exist does not. Even then, I wonder what keeps the buffer from being flushed after we return from XLogInsert() and before we set the bit, and if the answer is that nothing prevents that, whether that's OK. It might be good to talk about these issues too. Just to be clear, I'm not saying that I think the code is broken. But I am concerned about someone using this as precedent for code that runs in some other place, which would be highly likely to be broken, and the way to avoid that is for the comment to explain the tricky points. Also, you've named the parameter to this new function so that it's exactly the same as the global variable. I do approve of trying to pass the value as a parameter instead of relying on a global variable, and I wonder if you could find a way to remove the global variable entirely. But if not, I think the function parameter and the global variable should have different names, because otherwise it's easy for anyone reading the code to get confused about which one is being referenced in any particular spot, and it's also hard to grep. -- Robert Haas EDB: http://www.enterprisedb.com
