Hi Mark,
On Tue, Oct 07, 2025 at 04:55:53PM -0700, Mark Dilger wrote:
> v1-0001:
> All current callers of scan_rescan() pass NULL for the `key` parameter,
> making it unclear to
> Table AM authors if this is supposed to be a pointer to a single key, or
> an array. If an array,
> how is it terminated? None of this is addressed in the current code
> comments, and given
> that nobody uses this field, the intent is undiscoverable. By adding
> `int nkeys` to the parameter
> list, your patch makes the intention clearer. Perhaps you could also
> update the documentation
> for these functions?
>
> v1-0002
> Changing the EXPLAIN output to include where the exclusion happened is
> quite nice!
>
> Out of curiosity, why did you wait until this patch to add the `int
> nkeys` parameter to
> initscan()? It seems more on-topic for v1-0001. Likewise, renaming
> `key` as `keys` helps,
> but could have been done in v1-0001.
Thanks for your review. Please find a new version attached that addresses
these points.
> As for Andres' concern upstream, I am including a Work-In-Progress patch
> (WIP) to check
> that no light-weight locks are held during qual evaluation. I don't intend
> this for commit so much
> as for discussion. It seems to me fairly clear that your patch does not
> evaluate the quals while
> holding such locks, but I might have misunderstood the concern.
Thanks, it helps to confirm that due to the absence of ScanKey,
HeapKeyTest() is not called from heapgettup(), at least when running the
regression tests.
In order to guarantee to not hold the Buffer lock while evaluating a
ScanKey, v4-0001-Release-buffer-lock-before-scan-key-evaluation releases
the lock before calling HeapKeyTest().
Regards,
--
Julien Tachoires
