Thread: CLUSTER sort on abbreviated expressions is broken

Hi,

Independently of a problem with a recent commit, it seems that
$SUBJECT in all releases (well, I only tested as far back as 11).  I
attach an addition to the tests to show this, but here's a stand-alone
repro:

DROP TABLE IF EXISTS clstr_expression;

CREATE TABLE clstr_expression(id serial primary key, a int, b text COLLATE "C");
INSERT INTO clstr_expression(a, b) SELECT g.i % 42, 'prefix'||g.i FROM
generate_series(1, 133) g(i);
CREATE INDEX clstr_expression_minus_a ON clstr_expression ((-a), b);
CREATE INDEX clstr_expression_upper_b ON clstr_expression ((upper(b)));

CLUSTER clstr_expression USING clstr_expression_minus_a;
WITH rows AS
  (SELECT ctid, lag(a) OVER (ORDER BY ctid) AS la, a FROM clstr_expression)
SELECT * FROM rows WHERE la < a;

All good, and now for the part that I think is misbehaving:

CLUSTER clstr_expression USING clstr_expression_upper_b;
WITH rows AS
  (SELECT ctid, lag(b) OVER (ORDER BY ctid) AS lb, b FROM clstr_expression)
SELECT * FROM rows WHERE upper(lb) > upper(b);

That should produce no rows.  It works as expected if you SET
enable_seqscan = off and re-run CLUSTER, revealing that it's the
seq-scan-and-sort strategy that is broken.  It also works as expected
for non-yet-abbreviatable collations.

On Sun, Apr 3, 2022 at 11:05 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> Hi,
>
> Independently of a problem with a recent commit, it seems that
> $SUBJECT in all releases (well, I only tested as far back as 11).

I can confirm the problem on v10 as well.

-- 
John Naylor
EDB: http://www.enterprisedb.com

On Sun, Apr 3, 2022 at 8:22 PM John Naylor <john.naylor@enterprisedb.com> wrote:
> On Sun, Apr 3, 2022 at 11:05 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > Independently of a problem with a recent commit, it seems that
> > $SUBJECT in all releases (well, I only tested as far back as 11).
>
> I can confirm the problem on v10 as well.

Thanks for confirming.  I got as far as seeing that the two calls to
FormIndexDatum() are producing garbage in {l,r}_index_values, in the
loop at the end of comparetup_cluster(), but I'll have to come back to
this after some other stuff.

On Sun, Apr 3, 2022 at 1:22 AM John Naylor <john.naylor@enterprisedb.com> wrote:
> I can confirm the problem on v10 as well.

We will need a backpatchable fix, since Thomas' recent fix (commit
cc58eecc5d75a9329a6d49a25a6499aea7ee6fd6) only targeted the master
branch.

If we really needed the performance advantage of abbreviated keys in
this case then it would have taken more than 7 years for this bug to
come to light. The backpatchable fix can be very simple. We can just
copy what tuplesort_set_bound() does with abbreviated keys in
tuplesort_begin_cluster(), to explicitly disable abbreviated keys
up-front for affected tuplesorts. (Just for CLUSTER tuplesorts on an
expression index.)


-- 
Peter Geoghegan

On Mon, Apr 4, 2022 at 11:12 AM Peter Geoghegan <pg@bowt.ie> wrote:
> We will need a backpatchable fix, since Thomas' recent fix (commit
> cc58eecc5d75a9329a6d49a25a6499aea7ee6fd6) only targeted the master
> branch.

I probably should have made it clearer in the commit message,
cc58eecc5 doesn't fix this problem in the master branch.  It only
fixes the code that incorrectly assumed that datum1 was always
available.  Now it skips the optimised path, and falls back to the
slow path, that still has *this* bug, and the test upthread still
fails.  I wrote about this separately because it's clearly independent
and I didn't want it to be mistaken for an open item for 15.

On Sun, Apr 3, 2022 at 4:34 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> I probably should have made it clearer in the commit message,
> cc58eecc5 doesn't fix this problem in the master branch.  It only
> fixes the code that incorrectly assumed that datum1 was always
> available.

Attached patch fixes the issue, and includes the test case that you posted.

There is only a one line change to tuplesort.c. This is arguably the
same bug -- abbreviation is just another "haveDatum1 optimization"
that needs to be accounted for.

-- 
Peter Geoghegan

On Tue, Apr 12, 2022 at 11:01 AM Peter Geoghegan <pg@bowt.ie> wrote:
> Attached patch fixes the issue, and includes the test case that you posted.

Pushed a similar patch just now. Backpatched to all supported branches.

-- 
Peter Geoghegan

On Thu, Apr 21, 2022 at 12:18 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Tue, Apr 12, 2022 at 11:01 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > Attached patch fixes the issue, and includes the test case that you posted.
>
> Pushed a similar patch just now. Backpatched to all supported branches.

Thanks.