Re: Adding skip scan (including MDAM style range skip scan) to nbtree

> On 2024-11-21 17:47, Masahiro Ikeda wrote:
>> On 2024-11-21 04:40, Peter Geoghegan wrote:
>> diff --git a/src/backend/access/nbtree/nbtutils.c
>> b/src/backend/access/nbtree/nbtutils.c
>> index b70b58e0c..ddae5f2a1 100644
>> --- a/src/backend/access/nbtree/nbtutils.c
>> +++ b/src/backend/access/nbtree/nbtutils.c
>> @@ -3640,7 +3640,7 @@ _bt_advance_array_keys(IndexScanDesc scan,
>> BTReadPageState *pstate,
>>       * for skip scan, and stop maintaining the scan's skip arrays 
>> until we
>>       * reach the page's finaltup, if any.
>>       */
>> -    if (has_skip_array && beyond_end_advance &&
>> +    if (has_skip_array && !all_required_satisfied &&
>>          !has_required_opposite_direction_skip && pstate->finaltup)
>>          pstate->beyondskip = true;
>> 
>> However, a small number of my test cases now fail. And (I assume) this
>> approach has certain downsides on leaf pages where we're now too quick
>> to stop maintaining skip arrays.
> 
> Since I've built with the above change and executed make check, I found
> that there is an assertion error, which may not be related to what you 
> pointed
> out.
> 
> * the reproducible simple query (you can see the original query in
> opr_sanity.sql).
>   select * from pg_proc
>     where proname in (
>       'lo_lseek64',
>       'lo_truncate',
>       'lo_truncate64')
>     and pronamespace = 11;
> 
> * the assertion error
>   TRAP: failed Assert("sktrig_required && required"), File:
> "nbtutils.c", Line: 3375, PID: 362411
> 
> While investigating the error, I thought we might need to consider
> whether key->sk_flags does not have SK_BT_SKIP. The assertion error
> occurs because
> requiredSameDir doesn't become true since proname does not have 
> SK_BT_SKIP.
> 
> +        if (beyondskip)
> +        {
> +            /*
> +             * "Beyond end advancement" skip scan optimization.
> +             *
> +             * Just skip over any skip array scan keys.  Treat all other scan
> +             * keys as not required for the scan to continue.
> +             */
> +            Assert(!prechecked);
> +
> +            if (key->sk_flags & SK_BT_SKIP)
> +                continue;
> +        }
> +        else if (((key->sk_flags & SK_BT_REQFWD) && 
> ScanDirectionIsForward(dir)) ||
> +                 ((key->sk_flags & SK_BT_REQBKWD) && 
> ScanDirectionIsBackward(dir)))
>              requiredSameDir = true;
> 

My previous investigation was incorrect, sorry. IIUC, 
_bt_check_compare()
should return false as soon as possible with continuescan=true after the
tuple fails the key check when beyondskip=true, rather than setting
requiredSameDir to true. Because it has already been triggered to 
perform
a full index scan for the page.

Though the change fixes the assertion error in 'make check', there are 
still
cases where the number of return values is incorrect. I would also like 
to
continue investigating.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION