Re: BitmapHeapScan streaming read user and prelim refactoring

On 2/10/25 23:31, Melanie Plageman wrote:
> On Mon, Feb 10, 2025 at 4:22 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
>>
>> I don't really know what to do about this. The behavior of master
>> parallel bitmap heap scan can be emulated with the patch by increasing
>> effective_io_concurrency. But, IIRC we didn't want to do that for some
>> reason?
>> Not only does effective_io_concurrency == 1 negatively affect read
>> ahead, but it also prevents read combining regardless of the
>> io_combine_limit.
>
> Would a sensible default be something like 4?
>

Could be. It would perform much better for most systems I think, but
it's still a fairly conservative value so unlikely to cause regressions.

If we wanted to validate the value, what tests do you think would be
appropriate? What about just running the same tests with different eic
values, and pick a reasonable value based on that? One that helps, but
before it stats causing regressions.

FWIW I don't think we need to do this before pushing the patches, as
long as we do both. AFAIK there'll be an "apparent regression" no matter
the order of changes.

> I did some self-review on the patches and cleaned up the commit
> messages etc. Attached is v29.
>

I reviewed v29 today, and I think it's pretty much ready to go.

The one part where I don't quite get is 0001, which replaces a
FLEXIBLE_ARRAY_MEMBER array with a fixed-length array. It's not wrong,
but I don't quite see the benefits / clarity. And I think Thomas might
be right we may want to make this dynamic, to save memory.

Not a blocker, but I'd probably skip 0001 (unless it's required by the
later parts, I haven't checked/tried).

Other than that, I think this is ready.

regards

--
Tomas Vondra