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:
> 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.
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 82442db..5dae6d4 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -516,18 +516,10 @@ mdclose(SMgrRelation reln, ForkNumber forknum)
{
MdfdVec *v = &reln->md_seg_fds[forknum][nopensegs - 1];
- /* if not closed already */
- if (v->mdfd_vfd >= 0)
- {
- FileClose(v->mdfd_vfd);
- v->mdfd_vfd = -1;
- }
-
+ FileClose(v->mdfd_vfd);
+ _fdvec_resize(reln, forknum, nopensegs - 1);
nopensegs--;
}
-
- /* resize just once, avoids pointless reallocations */
- _fdvec_resize(reln, forknum, 0);
}
/*
@@ -1047,13 +1039,15 @@ _fdvec_resize(SMgrRelation reln,
reln->md_seg_fds[forknum] =
MemoryContextAlloc(MdCxt, sizeof(MdfdVec) * nseg);
}
- else
+ else if (nseg > reln->md_num_open_segs[forknum])
{
/*
* 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.
*/
reln->md_seg_fds[forknum] =
repalloc(reln->md_seg_fds[forknum],
