From: | "Bossart, Nathan" <bossartn(at)amazon(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Strange path from pgarch_readyXlog() |
Date: | 2021-12-30 16:50:53 |
Message-ID: | 307C1FF3-1019-4596-9D8D-7150C71DB74E@amazon.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 12/29/21, 3:11 PM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Bossart, Nathan" <bossartn(at)amazon(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Guillaume Lelarge | 2021-12-30 17:01:59 | Re: Autovacuum and idle_session_timeout |
Previous Message | Teodor Sigaev | 2021-12-30 16:40:09 | Pluggable toaster |