On 12/29/21, 3:11 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> 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.
Your assessment seems reasonable to me. If there was a better way to
adjust the comparator for the heap, maybe there would be a stronger
case for this approach. I certainly don't think it's worth inventing
something for just this use-case.
Thanks for fixing this!
Nathan