Re: Attempt to consolidate reading of XLOG page

From: Antonin Houska <ah(at)cybertec(dot)at>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Attempt to consolidate reading of XLOG page
Date: 2019-09-23 10:44:31
Message-ID: 46647.1569235471@antos
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

Attachment Content-Type Size
v06-0002-Remove-TLI-from-some-argument-lists.patch text/x-diff 10.8 KB
v06-0003-Introduce-WALOpenSegment-structure.patch text/x-diff 18.6 KB
v06-0004-Introduce-InvalidTimeLineID.patch text/x-diff 2.4 KB
v06-0005-Use-only-xlogreader.c-XLogRead.patch text/x-diff 18.0 KB
v06-0006-Remove-the-old-implemenations-of-XLogRead.patch text/x-diff 14.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2019-09-23 11:42:17 Re: Hi guys, HELP please
Previous Message Amit Kapila 2019-09-23 09:16:17 Re: pgbench - allow to create partitioned tables