On Wed, Nov 24, 2021 at 12:16 PM Heikki Linnakangas <hlinnaka@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