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
