Re: 2pc leaks fds
От | Alvaro Herrera |
---|---|
Тема | Re: 2pc leaks fds |
Дата | |
Msg-id | 20200507232855.GA28186@alvherre.pgsql обсуждение исходный текст |
Ответ на | Re: 2pc leaks fds (Kyotaro Horiguchi <horikyota.ntt@gmail.com>) |
Ответы |
Re: 2pc leaks fds
(Kyotaro Horiguchi <horikyota.ntt@gmail.com>)
|
Список | pgsql-hackers |
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. 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
Вложения
В списке pgsql-hackers по дате отправления: