I was confused by the struct name XLogSegment -- the struct is used to
represent a WAL segment while it's kept open, rather than just a WAL
segment in abstract. Also, now that we've renamed everything to use the
term WAL, it seems wrong to use the name XLog for new structs. I
propose the name WALOpenSegment for the struct, which solves both
problems. (Its initializer function would get the name
WALOpenSegmentInit.)
Now, the patch introduces a callback for XLogRead, the type of which is
called XLogOpenSegment. If we rename it from XLog to WAL, both names
end up the same. I propose to rename the function type to
WALSegmentOpen, which in a "noun-verb" view of the world, represents the
action of opening a WAL segment.
I attach a patch for all this renaming, on top of your series.
I wonder if each of those WALSegmentOpen callbacks should reset [at
least some members of] the struct; they're already in charge of setting
->file, and apparently we're leaving the responsibility of setting the
rest of the members to XLogRead. That seems weird. Maybe we should say
that the CB should only open the segment and not touch the struct at all
and XLogRead is in charge of everything. Perhaps the other way around
-- the CB should set everything correctly ... I'm not sure which is
best. But having half here and half there seems a recipe for confusion
and bugs.
Another thing I didn't like much is that everything seems to assume that
the only error possible from XLogRead is a read error. Maybe that's
okay, because it seems to be the current reality, but it seemed odd.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
