On 2021-Apr-07, Thomas Munro wrote:
> I wonder if it would be better to have the client code access these
> values through functions (even if they just access the variables in a
> static inline function), to create a bit more separation? Something
> like XLogReaderGetWanted(&page_lsn, &bytes_wanted), and then
> XLogReaderSetAvailable(state, 42)? Just an idea.
I think more opacity is good in this area, generally speaking. There
are way too many globals, and they interact in nontrivial ways across
the codebase. Just look at the ThisTimeLineID recent disaster. I
don't have this patch sufficiently paged-in to say that bytes_wanted/
bytes_available is precisely the thing we need, but if it makes for a
cleaner interface, I'm for it. This module keeps some state inside
itself, and others part of the state is in its users; that's not good,
and any cleanup on that is welcome.
BTRW it's funny that after these patches, "xlogreader" no longer reads
anything. It's more an "xlog interpreter" -- the piece of code that
splits individual WAL records from a stream of WAL bytes that's caller's
responsibility to obtain somehow. But (and, again, I haven't read this
patch recently) it still offers pieces that support a reader, in
addition to its main interface as the interpreter. Maybe it's not a
totally stupid idea to split it in even more different files.
--
Álvaro Herrera 39°49'30"S 73°17'W