On 12/29/21, 3:11 PM, "Tom Lane" <t...@sss.pgh.pa.us> wrote: > "Bossart, Nathan" <bossa...@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