Re: Attempt to consolidate reading of XLOG page

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:

> 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.

ok, thanks.

In addition I renamed WalSndOpenSegment() to WalSndSegmentOpen() and
read_local_xlog_page_open_segment() to read_local_xlog_page_segment_open()

> 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.

ok, I've changed the CB signature. Now it receives poiners to the two
variables that it can change while the "seg" argument is documented as
read-only. To indicate that the CB should determine timeline itself, I
introduced a new constant InvalidTimeLineID, see the 0004 part.

> 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.

In this case I only moved the ereport() code from XLogRead() away (so that
both backend and frontend can call the function). Given that the code to open
WAL segment is now in the callbacks, the only thing that XLogRead() can
ereport is that read() failed. BTW, I introduced one more structure,
XLogReadError, in this patch version. I think it's better than adding
error-specific fields to the WALOpenSegment structure.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com