"Bossart, Nathan" <bossa...@amazon.com> writes:
> On 12/29/21, 1:04 PM, "Tom Lane" <t...@sss.pgh.pa.us> wrote:
>> While we're here, I wonder if we ought to get rid of the static-ness of
>> these arrays. I realize that they're only eating a few kB, but they're
>> doing so in every postgres process, when they'll only be used in the
>> archiver.
> This crossed my mind, too. I also think one of the arrays can be
> eliminated in favor of just using the heap (after rebuilding with a
> reversed comparator). Here is a minimally-tested patch that
> demonstrates what I'm thinking.
I already pushed a patch that de-static-izes those arrays, so this
needs rebased at least. However, now that you mention it it does
seem like maybe the intermediate arch_files[] array could be dropped
in favor of just pulling the next file from the heap.
The need to reverse the heap's sort order seems like a problem
though. I really dislike the kluge you used here with a static flag
that inverts the comparator's sort order behind the back of the
binary-heap mechanism. It seems quite accidental that that doesn't
fall foul of asserts or optimizations in binaryheap.c. For
instance, if binaryheap_build decided it needn't do anything when
bh_has_heap_property is already true, this code would fail. In any
case, we'd need to spend O(n) time inverting the heap's sort order,
so this'd likely be slower than the current code.
On the whole I'm inclined not to bother trying to optimize this
further. The main thing that concerned me is that somebody would
bump up NUM_FILES_PER_DIRECTORY_SCAN to the point where the static
space consumption becomes really problematic, and we've fixed that.
regards, tom lane
