I have tested with and without the commit from Andres using the pgbench script (below) provided in the initial email.
pgbench -n -s 500 -c 4 -j 4 -T 100000 -P1 -f pgbench-write-2pc.sql I am not getting the leak anymore, it seems to be holding up pretty well. On Wed, Apr 8, 2020 at 12:59 PM Antonin Houska <a...@cybertec.at> wrote: > Andres Freund <and...@anarazel.de> wrote: > > > On 2020-04-06 09:12:32 +0200, Antonin Houska wrote: > > > Andres Freund <and...@anarazel.de> wrote: > > > It should have allowed users to have different ways to *locate the > segment* > > > file. The WALSegmentOpen callback could actually return file path > instead of > > > the file descriptor and let WALRead() perform the opening/closing, but > then > > > the WALRead function would need to be aware whether it is executing in > backend > > > or in frontend (so it can use the correct function to open/close the > file). > > > > > > I was aware of the problem that the correct function should be used to > open > > > the file and that's why this comment was added (although "mandatory" > would be > > > more suitable than "preferred"): > > > > > > * BasicOpenFile() is the preferred way to open the segment file in > backend > > > * code, whereas open(2) should be used in frontend. > > > */ > > > typedef int (*WALSegmentOpen) (XLogSegNo nextSegNo, WALSegmentContext > *segcxt, > > > TimeLineID > *tli_p); > > > > I don't think that BasicOpenFile() really solves anything here? If > > anything it *exascerbates* the problem, because it will trigger all of > > the "virtual file descriptors" for already opened Files to close() the > > underlying OS FDs. So not even a fully cached table can be seqscanned, > > because that tries to check the file size... > > Specifically for 2PC, isn't it better to close some OS-level FD of an > unrelated table scan and then succeed than to ERROR immediately? Anyway, > 0dc8ead46 hasn't changed this. > > I at least admit that the comment should not recommend particular function, > and that WALRead() should call the appropriate function to close the file, > rather than always calling close(). > > Can we just pass the existing FD to the callback as an additional argument, > and let the callback close it? > > -- > Antonin Houska > Web: https://www.cybertec-postgresql.com > > > -- Highgo Software (Canada/China/Pakistan) URL : http://www.highgo.ca ADDR: 10318 WHALLEY BLVD, Surrey, BC EMAIL: mailto: ahsan.h...@highgo.ca
