Re: 2pc leaks fds

At Thu, 7 May 2020 19:28:55 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in 
> On 2020-Apr-27, Kyotaro Horiguchi wrote:
> 
> > At Fri, 24 Apr 2020 11:48:46 -0400, Alvaro Herrera <alvherre@2ndquadrant.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.

Agreed.

> 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 agree that new callback functions is most sensible for getting into
13, of course.

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

Thanks.

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

I agree to the direction of this patch. Thanks for the explanation.
The patch looks good to me except the two points below.


+	/* XXX for xlogreader use, we'd call XLogBeginRead+XLogReadRecord here */
+	if (!WALRead(&fake_xlogreader,
+				 &output_message.data[output_message.len],

I'm not sure the point of the XXX comment, but I think WALRead here is
the right thing and we aren't going to use
XLogBeginRead+XLogReadRecord here.  So it seems to me the comment is
misleading and instead we need such a comment for fake_xlogreader like
this.

+	static XLogReaderState fake_xlogreader =
+	{
+		/* fake reader state only to let WALRead to use the callbacks */


wal_segment_close(XlogReaderState *state) is setting
state->seg.ws_file to -1.  On the other hand wal_segment_close(state,..)
doesn't update ws_file and the caller sets the returned value to
(eventually) the same field.

+			seg->ws_file = state->routine.segment_open(state, nextSegNo,
+													   segcxt, &tli);

If you are willing to do so, I think it is better to make the callback
functions are responsible to update the seg.ws_file and the callers
don't care.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center