Re: 2pc leaks fds

On 2020-Apr-27, Kyotaro Horiguchi wrote:

> At Fri, 24 Apr 2020 11:48:46 -0400, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote in

> Sorry for the ambiguity, I didn't meant I minded that this conflicts
> with my patch or I don't want this to be committed. It is easily
> rebased on this patch. What I was anxious about is that the new
> callback struct might be too flexible than required. So I "mildly"
> objected, and I won't be dissapointed if this patch is committed.

... well, yeah, maybe it is too flexible. And perhaps we could further
tweak this interface so that the file descriptor is not part of
XLogReader at all -- with such a change, it would make more sense to
worry about the "close" callback not being "close" but something like
"cleanup", as you suggest. But right now, and thinking from the point
of view of going into postgres 13 beta shortly, it seems to me that
XLogReader is just a very leaky abstraction since both itself and its
users are aware of the fact that there is a file descriptor.

Maybe with your rework for encryption you'll want to remove the FD from
XLogReader at all, and move it elsewhere. Or maybe not. But it seems
to me that my suggested approach is sensible, and better than the
current situation. (Let's keep in mind that the primary concern here is
that the callstack is way too complicated -- you ask XlogReader for
data, it calls your Read callback, that one calls WALRead passing your
openSegment callback and stuffs the FD in XLogReaderState ... a sieve it
is, the way it leaks, not an abstraction.)

> > I have to admit that until today I hadn't realized that that's what your
> > patch series was doing. I'm not familiar with how you intend to
> > implement WAL encryption on top of this, but on first blush I'm not
> > liking this proposed design too much.
>
> Right. I might be too much in detail, but it simplifies the call
> tree. Anyway that is another discussion, though:)

Okay. We can discuss further changes later, of course.

> It looks like as if the open/read/close-callbacks are generic and on
> the same interface layer, but actually open-callback is dedicate to
> WALRead and it is useless when the read-callback doesn't use
> WALRead. What I was anxious about is that the open-callback is
> uselessly exposing the secret of the read-callback.

Well, I don't think we care about that. WALRead can be thought of as
just a helper function that you may use to write your read callback.
The comments I added explain this.

> I meant concretely that we only have read- and cleanup- callbacks in
> xlogreader state. The caller of XLogReaderAllocate specifies the
> cleanup-callback that is to be used to clean up what the
> reader-callback left behind, in the same manner with this patch does.
> The only reason it is not named close-callback is that it is used only
> when the xlogreader-state is destroyed. So I'm fine with
> read/close-callbacks.

We can revisit the current design in the future. For example for
encryption we might decide to remove the current-open-segment FD from
XLogReaderState and then things might be different. (I think the
current design is based a lot on historical code, rather than being
optimal.)

Since your objection isn't strong, I propose to commit same patch as
before, only rebased as conflicted with cd123234404e and this comment
prologuing WALRead:

/*
* Helper function to ease writing of XLogRoutine->page_read callbacks.
* If this function is used, caller must supply an open_segment callback in
* 'state', as that is used here.
[... rest is same as before ...]

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services