Re: mdclose() does not cope w/ FileClose() failure

Hello.

At Sun, 22 Dec 2019 12:21:00 -0800, Noah Misch <noah@leadboat.com> wrote in 
> On Sun, Dec 22, 2019 at 01:19:30AM -0800, Noah Misch wrote:
> > I am inclined to fix this by decrementing md_num_open_segs before modifying
> > md_seg_fds (second attachment).
> 
> That leaked memory, since _fdvec_resize() assumes md_num_open_segs is also the
> allocated array length.  The alternative is looking better:

I agree that v2 is cleaner in the light of modularity and fixes the
memory leak happens at re-open.

> > An alternative would be to call
> > _fdvec_resize() after every FileClose(), like mdtruncate() does; however, the
> > repalloc() overhead could be noticeable.  (mdclose() is called much more
> > frequently than mdtruncate().)
> 
> I can skip repalloc() when the array length decreases, to assuage mdclose()'s
> worry.  In the mdclose() case, the final _fdvec_resize(reln, fork, 0) will
> still pfree() the array.  Array elements that mdtruncate() frees today will
> instead persist to end of transaction.  That is okay, since mdtruncate()
> crossing more than one segment boundary is fairly infrequent.  For it to
> happen, you must either create a >2G relation and then TRUNCATE it in the same
> transaction, or VACUUM must find >1-2G of unused space at the end of the
> relation.  I'm now inclined to do it that way, attached.

         * It doesn't seem worthwhile complicating the code by having a more
         * aggressive growth strategy here; the number of segments doesn't
         * grow that fast, and the memory context internally will sometimes
-         * avoid doing an actual reallocation.
+         * avoid doing an actual reallocation.  Likewise, since the number of
+         * segments doesn't shrink that fast, don't shrink at all.  During
+         * mdclose(), we'll pfree the array at nseg==0.

If I understand it correctly, it is mentioning the number of the all
segment files in a fork, not the length of md_seg_fds arrays at a
certain moment. But actually _fdvec_resize is called for every segment
opening during mdnblocks (just-after mdopen), and every segment
closing during mdclose and mdtruncate as mentioned here. We are going
to omit pallocs only in the decreasing case.

If we regard repalloc as far faster than FileOpen/FileClose or we care
about only increase of segment number of mdopen'ed files and don't
care the frequent resize that happens during the functions above, then
the comment is right and we may resize the array in the
segment-by-segment manner.

But if they are comparable each other, or we don't want the array gets
resized frequently, we might need to prevent repalloc from happening
on every segment increase, too.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center