Обсуждение: BUG #19049: Assert failure when using skip arrays on an index key with DESC order

The following bug has been logged on the website:

Bug reference:      19049
Logged by:          Natalya Aksman
Email address:      natalya@timescale.com
PostgreSQL version: 18rc1
Operating system:   Ubuntu 24.04
Description:

Reproducer:

-- Set up a table with >=2 DESC index keys
create table mv (t1 int, t2 int, s0 int, s1 int);
create index on mv(s0, s1, t1 desc, t2 desc);
insert into mv select f.t,  f.t, 1 s0, 1 s1 from generate_series(1, 2 *
pow(10, 4)::int) f(t);
analyze mv;

-- Make sure index is used
set enable_seqscan=0;
explain (costs off) select distinct on(s0, s1) * from mv where t2 >= -1 and
t1 < 10 order by s0, s1, t1 desc, t2 desc;
                        QUERY PLAN
-----------------------------------------------------------
 Unique
   ->  Index Only Scan using mv_s0_s1_t1_t2_idx on mv
         Index Cond: ((t1 < 10) AND (t2 >= '-1'::integer))
(3 rows)

-- This query crashes with an Assert
 select distinct on(s0, s1) * from mv where t2 >= -1 and t1 < 10 order by
s0, s1, t1 desc, t2 desc;

Assert is:
                if (ScanDirectionIsForward(dir) && array->low_compare)
Assert(DatumGetBool(FunctionCall2Coll(&array->low_compare->sk_func,
array->low_compare->sk_collation,
tupdatum,
array->low_compare->sk_argument)));

The problem is "array->low_compare" changing "t1<10" to "t1>10" because t1
order is DESC.

PG Bug reporting form <noreply@postgresql.org> writes:
> -- This query crashes with an Assert
>  select distinct on(s0, s1) * from mv where t2 >= -1 and t1 < 10 order by
> s0, s1, t1 desc, t2 desc;

> Assert is:
>                 if (ScanDirectionIsForward(dir) && array->low_compare)
> Assert(DatumGetBool(FunctionCall2Coll(&array->low_compare->sk_func,
> array->low_compare->sk_collation, tupdatum, array->low_compare->sk_argument)));

Indeed.  "git bisect" says this started at

b3f1a13f22f9e28842ee5fbd08b7ec805e27aaac is the first bad commit
commit b3f1a13f22f9e28842ee5fbd08b7ec805e27aaac
Author: Peter Geoghegan <pg@bowt.ie>
Date:   Fri Apr 4 14:14:08 2025 -0400

    Avoid extra index searches through preprocessing.

Peter, you want to take a look?

            regards, tom lane

On Thu, Sep 11, 2025 at 11:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter, you want to take a look?

Looking at it now...

--
Peter Geoghegan

On Thu, Sep 11, 2025 at 10:35 AM PG Bug reporting form
<noreply@postgresql.org> wrote:
> Assert is:
>                 if (ScanDirectionIsForward(dir) && array->low_compare)
> Assert(DatumGetBool(FunctionCall2Coll(&array->low_compare->sk_func,
> array->low_compare->sk_collation,
> tupdatum,
> array->low_compare->sk_argument)));
>
> The problem is "array->low_compare" changing "t1<10" to "t1>10" because t1
> order is DESC.

This explanation is correct: I simply neglected to account for the way
that DESC column scalar keys (used as a skip array's
low_compare/high_compare) will have already had their operator
strategy inverted, at the point where preprocessing converts a
low_compare > into an equivalent >= (and/or a high_compare < into an
equivalent <=) as an optimization. As Tom pointed out, this is a bug
in the mechanism added by commit b3f1a13f (not the main skip scan
commit)

A draft fix is attached. This fixes the crash that your test case
exhibits, and passes all of my existing tests. I'll commit something
along these lines within the next few days, barring any objections.

I'm going to revisit my test coverage for DESC columns in light of
this issue. I didn't add support for DESC columns in my fuzz testing
scripts, which now seems like an oversight -- I'd definitely have
caught this problem myself, had I done that from the start.

--
Peter Geoghegan

On Thu, Sep 11, 2025 at 1:09 PM Natalya Aksman <natalya@tigerdata.com> wrote:
> The patch looks good and fixes our failing tests.

Thanks for the high quality bug report!

--
Peter Geoghegan

On Thu, Sep 11, 2025 at 12:48 PM Peter Geoghegan <pg@bowt.ie> wrote:
> I'm going to revisit my test coverage for DESC columns in light of
> this issue. I didn't add support for DESC columns in my fuzz testing
> scripts, which now seems like an oversight -- I'd definitely have
> caught this problem myself, had I done that from the start.

Just for the record, I tried this out. Here's what happened:

I found that when I introduced alternating ASC/DESC column orders to
my existing fuzz testing script, it found the same bug in literally a
fraction of a second. I didn't see the same assertion failure that
Natalya's test case ran into, though; the script just detected
discrepancies between results from an index scan, and an equivalent
sequential scan (both for the same randomly generated query).

I also found that with my patch was applied, the same fuzz testing
script correctly executed several million randomly generated test
queries without incident. This includes randomly generated
combinations of equality/inequality operators, IS NULL/IS NOT NULL
conditions, IN() constructs, and skip arrays. All against either one
of a pair of indexes, each with 4 index columns (now with alternating
ASC and DESC column orders) -- there are two indexes to cover
different permutations of NULLS FIRST and NULLS LAST. The fuzz testing
script generates both backwards and forwards scans, for both of these
multicolumn indexes.

Obviously, this doesn't prove the absence of more DESC column related
bugs in the skip scan patchset. But it does make me confident that any
remaining DESC column related bugs are bound to be far more
subtle/hard to hit than this one was.

--
Peter Geoghegan