On 22/02/2023 15:03, Aleksander Alekseev wrote:
> Additionally, practice shows that for an extension author it's very
> easy to miss a comment in skey.h:
>
> """
> * SK_SEARCHARRAY, SK_SEARCHNULL and SK_SEARCHNOTNULL are supported only
> * for index scans, not heap scans;
> """
>
> ... which results in many hours of debugging. The current interface is
> misleading and counterintuitive.
Perhaps an Assert in heap_beginscan would be in order, to check that
none of those flags are set.
>> But then let's also modify the caller in nodeSeqScan.c to actually make use of it.
>
> That could actually be a good point.
>
> If memory serves I noticed that WHERE ... IS NULL queries don't even
> hit HeapKeyTest() and I was curious where the check for NULLs is
> actually made. As I understand, SeqNext() in nodeSeqscan.c simply
> iterates over all the tuples it can find and pushes them to the parent
> node. We could get a slightly better performance for certain queries
> if SeqNext() did the check internally.
Right, it might be faster to perform the NULL-checks before checking
visibility, for example. Arbitrary quals cannot be evaluated before
checking visibility, but NULL checks could be.
> Unfortunately I'm not very experienced with plan nodes in order to go
> down this rabbit hole straight away. I suggest we make one change at a
> time and keep the patchset small as it was previously requested by
> many people on several occasions (the 64-bit XIDs story, etc). I will
> be happy to propose a follow-up patch accompanied by the benchmarks if
> and when we reach the consensus on this patch.
Ok, I don't think this patch on its own is a good idea, without the
other parts, so I'll mark this as Returned with Feedback in the commitfest.
- Heikki