Re: Making Row Comparison NULL row member handling more robust during skip scans

On 01/07/2025 19:37, Peter Geoghegan wrote:
> On Fri, Jun 27, 2025 at 5:35 PM Peter Geoghegan <pg@bowt.ie> wrote:
>> Attached is v4, which is largely just a polished version of v3. It has
>> improved comments and more worked out commit messages. Plus the second
>> patch (the row compare patch) now teaches _bt_first to fully rely on
>> scan key requiredness markings, just like with other scan keys.
>
> Heikki said he'd be able to give this patch set at least a quick
> review, so here's a new revision, v4.
>
> This isn't really different to v4. It has more comment cleanup, and
> better commit messages.

I like how this makes row comparisons less special. To be honest I don't
quite understand what the bug was, I'd have to dig much deeper into this
to swap enough context into my memory for that. But just reading the
comments in these patches, I get the impression that they make this all
less fragile.

On bt_unmark_extra_keys() function: The function comment explains well
what it does; that's good. A few very minor stylistic remarks on its
implementation:

 > +    /* Set things up for first key's attr */
 > +    origkey = so->keyData;
 > +    curattr = origkey->sk_attno;
 > +    firsti = 0;
 > +    haveReqEquals = false;
 > +    haveReqForward = false;
 > +    haveReqBackward = false;
 > +    for (int i = 0; i < so->numberOfKeys; origkey++, i++)
 > +    {

It took me a moment to understand that origkey is always pointing to
&so->keyData[i]. I'd suggest making it more clear with:

    /* Set things up for first key's attr */
    curattr = so->keyData[0].sk_attno;
    firsti = 0;
    haveReqEquals = false;
    haveReqForward = false;
    haveReqBackward = false;
    for (int i = 0; i < so->numberOfKeys; i++)
    {
        ScanKey        origkey = &so->keyData[i];

In the second loop in the function you're actually doing this:

 > +        origkey = so->keyData + i;

which is yet another way to say the same thing. IMHO "&so->keyData[i]"
is more readable.

Would it be worth adding a comment that we assume keyData to be in
sk_attno order? Or is that a widely-known assumption? I don't remember.

 > +    /*
 > +     * Next, allocate temp arrays: one set for unchanged keys, another for
 > +     * keys that will be unmarked/made non-required
 > +     */
 > +    unmarkKeys = palloc(so->numberOfKeys * sizeof(ScanKeyData));
 > +    keepKeys = palloc(so->numberOfKeys * sizeof(ScanKeyData));
Doesn't matter much but I think this could be:

    unmarkKeys = palloc(nunmark * sizeof(ScanKeyData));
    keepKeys = palloc((so->numberOfKeys - nunmark) * sizeof(ScanKeyData));

Or use a single array, putting the unmarked entries to the end of the array.

PS. Not a new thing, but the "Add coverage..." comments in the tests
seem redundant. All tests add coverage for something.

- Heikki