Skip to content

Commit 4454f27

Browse files
svenklemmtimescale-automation
svenklemm
authored and
timescale-automation
committed
Fix first/last optimization with different orderings
When a query used two first() or two last() calls on the same value column but ordered by different columns, both calls returned the same value. The optimization that turns first/last into an index scan matched the calls only by the value column and ignored the ordering column, so both ended up reusing the same subquery result. The ordering column is now part of the match, so each call keeps its own result. (cherry picked from commit 4ed88d2)
1 parent ffd61eb commit 4454f27

7 files changed

Lines changed: 584 additions & 80 deletions

File tree

‎.unreleased/fix_10060‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fixes: #10060 Fix first/last optimization returning the same value for aggregates that share the value column but order by different columns

‎src/planner/agg_bookend.c‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -71,19 +71,20 @@ typedef struct FirstLastAggInfo
7171
MinMaxAggInfo *m_agg_info; /* reusing MinMaxAggInfo to avoid code
7272
* duplication */
7373
Expr *sort; /* Expression to use for ORDER BY */
74+
Aggref *aggref; /* Aggref node for this aggregate */
7475
} FirstLastAggInfo;
7576

7677
typedef struct MutatorContext
7778
{
78-
MinMaxAggPath *mm_path;
79+
List *first_last_aggs;
7980
} MutatorContext;
8081

8182
static bool find_first_last_aggs_walker(Node *node, List **context);
8283
static bool build_first_last_path(PlannerInfo *root, FirstLastAggInfo *fl_info, Oid eqop,
8384
Oid sortop, bool reverse_sort, bool nulls_first);
8485
static void first_last_qp_callback(PlannerInfo *root, void *extra);
8586
static Node *mutate_aggref_node(Node *node, MutatorContext *context);
86-
static void replace_aggref_in_tlist(MinMaxAggPath *minmaxagg_path);
87+
static void replace_aggref_in_tlist(MinMaxAggPath *minmaxagg_path, List *first_last_aggs);
8788

8889
/*
8990
* mutate_aggref_node
@@ -103,16 +104,17 @@ mutate_aggref_node(Node *node, MutatorContext *context)
103104
Aggref *aggref = (Aggref *) node;
104105

105106
/* See if the Aggref should be replaced by a Param */
106-
if (context->mm_path != NULL && list_length(aggref->args) == 2)
107+
if (list_length(aggref->args) == 2)
107108
{
108-
TargetEntry *curTarget = (TargetEntry *) linitial(aggref->args);
109109
ListCell *cell;
110110

111-
foreach (cell, context->mm_path->mmaggregates)
111+
/* match on aggregate, value and sort expression */
112+
foreach (cell, context->first_last_aggs)
112113
{
113-
MinMaxAggInfo *mminfo = (MinMaxAggInfo *) lfirst(cell);
114+
FirstLastAggInfo *fl_info = (FirstLastAggInfo *) lfirst(cell);
115+
MinMaxAggInfo *mminfo = fl_info->m_agg_info;
114116

115-
if (mminfo->aggfnoid == aggref->aggfnoid && equal(mminfo->target, curTarget->expr))
117+
if (equal(fl_info->aggref, aggref))
116118
{
117119
return (Node *) copyObject(mminfo->param);
118120
}
@@ -132,11 +134,11 @@ mutate_aggref_node(Node *node, MutatorContext *context)
132134
*
133135
*/
134136
void
135-
replace_aggref_in_tlist(MinMaxAggPath *minmaxagg_path)
137+
replace_aggref_in_tlist(MinMaxAggPath *minmaxagg_path, List *first_last_aggs)
136138
{
137139
MutatorContext context;
138140

139-
context.mm_path = minmaxagg_path;
141+
context.first_last_aggs = first_last_aggs;
140142

141143
((Path *) minmaxagg_path)->pathtarget->exprs =
142144
(List *) mutate_aggref_node((Node *) ((Path *) minmaxagg_path)->pathtarget->exprs,
@@ -251,16 +253,18 @@ ts_preprocess_first_last_aggregates(PlannerInfo *root, List *tlist)
251253
/*
252254
* Reject unoptimizable cases.
253255
*
254-
* We don't handle the case when agg function is in ORDER BY. The reason
255-
* being is that we replace Aggref node before sort keys are being
256-
* generated.
256+
* We don't handle the case when a first/last function is in ORDER BY or
257+
* DISTINCT because we replace the Aggref node before sort keys are being
258+
* generated, so the sort keys could not be matched to the modified path
259+
* target.
257260
*
258261
* We don't handle GROUP BY or windowing, because our current
259262
* implementations of grouping require looking at all the rows anyway, and
260263
* so there's not much point in optimizing FIRST/LAST.
261264
*/
262265
if (parse->groupClause || list_length(parse->groupingSets) > 1 || parse->hasWindowFuncs ||
263-
contains_first_last_node(parse->sortClause, tlist))
266+
contains_first_last_node(parse->sortClause, tlist) ||
267+
contains_first_last_node(parse->distinctClause, tlist))
264268
{
265269
return;
266270
}
@@ -413,7 +417,7 @@ ts_preprocess_first_last_aggregates(PlannerInfo *root, List *tlist)
413417
mm_agg_list,
414418
(List *) parse->havingQual);
415419
/* Let's replace Aggref node since we will use subquery we've generated */
416-
replace_aggref_in_tlist(minmaxagg_path);
420+
replace_aggref_in_tlist(minmaxagg_path, first_last_aggs);
417421
add_path(grouped_rel, (Path *) minmaxagg_path);
418422
}
419423

@@ -532,8 +536,11 @@ find_first_last_aggs_walker(Node *node, List **context)
532536
*/
533537
foreach (l, *context)
534538
{
535-
mminfo = (MinMaxAggInfo *) lfirst(l);
536-
if (mminfo->aggfnoid == aggref->aggfnoid && equal(mminfo->target, value->expr))
539+
FirstLastAggInfo *existing = (FirstLastAggInfo *) lfirst(l);
540+
541+
mminfo = existing->m_agg_info;
542+
if (mminfo->aggfnoid == aggref->aggfnoid && equal(mminfo->target, value->expr) &&
543+
equal(existing->sort, sort->expr))
537544
{
538545
return false;
539546
}
@@ -551,6 +558,7 @@ find_first_last_aggs_walker(Node *node, List **context)
551558
fl_info = palloc(sizeof(FirstLastAggInfo));
552559

553560
fl_info->m_agg_info = mminfo;
561+
fl_info->aggref = aggref;
554562
fl_info->sort = sort->expr;
555563
*context = lappend(*context, fl_info);
556564

‎test/expected/agg_bookends-15.out‎

Lines changed: 136 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,56 @@ CREATE INDEX btest_time_alt_idx ON btest(time_alt);
581581
-> Seq Scan on _hyper_1_4_chunk (actual rows=1.00 loops=1)
582582
-> Seq Scan on _hyper_1_5_chunk (actual rows=1.00 loops=1)
583583

584+
ROLLBACK;
585+
-- two FIRST/LAST sharing the value column but ordering on different columns
586+
BEGIN;
587+
CREATE TABLE bookend_two_orderings(time timestamptz NOT NULL, time_alt timestamptz NOT NULL, val int NOT NULL);
588+
SELECT schema_name, table_name, created FROM create_hypertable('bookend_two_orderings', 'time');
589+
schema_name | table_name | created
590+
-------------+-----------------------+---------
591+
public | bookend_two_orderings | t
592+
593+
INSERT INTO bookend_two_orderings
594+
SELECT '2025-01-01'::timestamptz + g * interval '1 minute',
595+
'2025-01-01'::timestamptz + (201 - g) * interval '1 minute',
596+
g
597+
FROM generate_series(1, 200) g;
598+
CREATE INDEX ON bookend_two_orderings(time);
599+
CREATE INDEX ON bookend_two_orderings(time_alt);
600+
:PREFIX SELECT first(val, time), first(val, time_alt) FROM bookend_two_orderings;
601+
--- QUERY PLAN ---
602+
Result (actual rows=1.00 loops=1)
603+
InitPlan 1 (returns $1)
604+
-> Limit (actual rows=1.00 loops=1)
605+
-> Index Scan using _hyper_3_8_chunk_bookend_two_orderings_time_alt_idx on _hyper_3_8_chunk (actual rows=1.00 loops=1)
606+
InitPlan 2 (returns $0)
607+
-> Limit (actual rows=1.00 loops=1)
608+
-> Index Scan using _hyper_3_8_chunk_bookend_two_orderings_time_idx1 on _hyper_3_8_chunk _hyper_3_8_chunk_1 (actual rows=1.00 loops=1)
609+
610+
:PREFIX SELECT last(val, time), last(val, time_alt) FROM bookend_two_orderings;
611+
--- QUERY PLAN ---
612+
Result (actual rows=1.00 loops=1)
613+
InitPlan 1 (returns $1)
614+
-> Limit (actual rows=1.00 loops=1)
615+
-> Index Scan Backward using _hyper_3_8_chunk_bookend_two_orderings_time_alt_idx on _hyper_3_8_chunk (actual rows=1.00 loops=1)
616+
Index Cond: (time_alt IS NOT NULL)
617+
InitPlan 2 (returns $0)
618+
-> Limit (actual rows=1.00 loops=1)
619+
-> Index Scan Backward using _hyper_3_8_chunk_bookend_two_orderings_time_idx1 on _hyper_3_8_chunk _hyper_3_8_chunk_1 (actual rows=1.00 loops=1)
620+
Index Cond: ("time" IS NOT NULL)
621+
622+
-- DISTINCT sorts on the first/last results, so the optimization must be skipped
623+
SET enable_hashagg = off;
624+
:PREFIX SELECT DISTINCT first(val, time), first(val, time_alt) FROM bookend_two_orderings;
625+
--- QUERY PLAN ---
626+
Unique (actual rows=1.00 loops=1)
627+
-> Sort (actual rows=1.00 loops=1)
628+
Sort Key: (first(_hyper_3_8_chunk.val, _hyper_3_8_chunk."time")), (first(_hyper_3_8_chunk.val, _hyper_3_8_chunk.time_alt))
629+
Sort Method: quicksort
630+
-> Aggregate (actual rows=1.00 loops=1)
631+
-> Seq Scan on _hyper_3_8_chunk (actual rows=200.00 loops=1)
632+
633+
RESET enable_hashagg;
584634
ROLLBACK;
585635
-- Test with NULL numeric values
586636
BEGIN;
@@ -606,25 +656,25 @@ INSERT INTO btest_numeric VALUES('2018-01-20T09:00:43', NULL);
606656
Result (actual rows=1.00 loops=1)
607657
InitPlan 1 (returns $0)
608658
-> Limit (actual rows=1.00 loops=1)
609-
-> Index Scan Backward using _hyper_2_8_chunk_btest_numeric_time_idx on _hyper_2_8_chunk (actual rows=1.00 loops=1)
659+
-> Index Scan Backward using _hyper_2_9_chunk_btest_numeric_time_idx on _hyper_2_9_chunk (actual rows=1.00 loops=1)
610660

611661
:PREFIX SELECT last(quantity, time) FROM btest_numeric;
612662
--- QUERY PLAN ---
613663
Result (actual rows=1.00 loops=1)
614664
InitPlan 1 (returns $0)
615665
-> Limit (actual rows=1.00 loops=1)
616-
-> Index Scan using _hyper_2_8_chunk_btest_numeric_time_idx on _hyper_2_8_chunk (actual rows=1.00 loops=1)
666+
-> Index Scan using _hyper_2_9_chunk_btest_numeric_time_idx on _hyper_2_9_chunk (actual rows=1.00 loops=1)
617667
Index Cond: ("time" IS NOT NULL)
618668

619669
:PREFIX SELECT first(time, quantity) FROM btest_numeric;
620670
--- QUERY PLAN ---
621671
Aggregate (actual rows=1.00 loops=1)
622-
-> Seq Scan on _hyper_2_8_chunk (actual rows=2.00 loops=1)
672+
-> Seq Scan on _hyper_2_9_chunk (actual rows=2.00 loops=1)
623673

624674
:PREFIX SELECT last(time, quantity) FROM btest_numeric;
625675
--- QUERY PLAN ---
626676
Aggregate (actual rows=1.00 loops=1)
627-
-> Seq Scan on _hyper_2_8_chunk (actual rows=2.00 loops=1)
677+
-> Seq Scan on _hyper_2_9_chunk (actual rows=2.00 loops=1)
628678

629679
-- NULL values followed by non-NULL values
630680
INSERT INTO btest_numeric VALUES('2019-01-20T09:00:43', 1);
@@ -636,8 +686,8 @@ INSERT INTO btest_numeric VALUES('2019-01-20T09:00:43', 2);
636686
-> Limit (actual rows=1.00 loops=1)
637687
-> Custom Scan (ChunkAppend) on btest_numeric (actual rows=1.00 loops=1)
638688
Order: btest_numeric."time"
639-
-> Index Scan Backward using _hyper_2_8_chunk_btest_numeric_time_idx on _hyper_2_8_chunk (actual rows=1.00 loops=1)
640-
-> Index Scan Backward using _hyper_2_9_chunk_btest_numeric_time_idx on _hyper_2_9_chunk (never executed)
689+
-> Index Scan Backward using _hyper_2_9_chunk_btest_numeric_time_idx on _hyper_2_9_chunk (actual rows=1.00 loops=1)
690+
-> Index Scan Backward using _hyper_2_10_chunk_btest_numeric_time_idx on _hyper_2_10_chunk (never executed)
641691

642692
:PREFIX SELECT last(quantity, time) FROM btest_numeric;
643693
--- QUERY PLAN ---
@@ -646,24 +696,24 @@ INSERT INTO btest_numeric VALUES('2019-01-20T09:00:43', 2);
646696
-> Limit (actual rows=1.00 loops=1)
647697
-> Custom Scan (ChunkAppend) on btest_numeric (actual rows=1.00 loops=1)
648698
Order: btest_numeric."time" DESC
649-
-> Index Scan using _hyper_2_9_chunk_btest_numeric_time_idx on _hyper_2_9_chunk (actual rows=1.00 loops=1)
699+
-> Index Scan using _hyper_2_10_chunk_btest_numeric_time_idx on _hyper_2_10_chunk (actual rows=1.00 loops=1)
650700
Index Cond: ("time" IS NOT NULL)
651-
-> Index Scan using _hyper_2_8_chunk_btest_numeric_time_idx on _hyper_2_8_chunk (never executed)
701+
-> Index Scan using _hyper_2_9_chunk_btest_numeric_time_idx on _hyper_2_9_chunk (never executed)
652702
Index Cond: ("time" IS NOT NULL)
653703

654704
:PREFIX SELECT first(time, quantity) FROM btest_numeric;
655705
--- QUERY PLAN ---
656706
Aggregate (actual rows=1.00 loops=1)
657707
-> Append (actual rows=4.00 loops=1)
658-
-> Seq Scan on _hyper_2_8_chunk (actual rows=2.00 loops=1)
659708
-> Seq Scan on _hyper_2_9_chunk (actual rows=2.00 loops=1)
709+
-> Seq Scan on _hyper_2_10_chunk (actual rows=2.00 loops=1)
660710

661711
:PREFIX SELECT last(time, quantity) FROM btest_numeric;
662712
--- QUERY PLAN ---
663713
Aggregate (actual rows=1.00 loops=1)
664714
-> Append (actual rows=4.00 loops=1)
665-
-> Seq Scan on _hyper_2_8_chunk (actual rows=2.00 loops=1)
666715
-> Seq Scan on _hyper_2_9_chunk (actual rows=2.00 loops=1)
716+
-> Seq Scan on _hyper_2_10_chunk (actual rows=2.00 loops=1)
667717

668718
TRUNCATE btest_numeric;
669719
-- non-NULL values followed by NULL values
@@ -678,8 +728,8 @@ INSERT INTO btest_numeric VALUES('2018-01-20T09:00:43', NULL);
678728
-> Limit (actual rows=1.00 loops=1)
679729
-> Custom Scan (ChunkAppend) on btest_numeric (actual rows=1.00 loops=1)
680730
Order: btest_numeric."time"
681-
-> Index Scan Backward using _hyper_2_11_chunk_btest_numeric_time_idx on _hyper_2_11_chunk (actual rows=1.00 loops=1)
682-
-> Index Scan Backward using _hyper_2_10_chunk_btest_numeric_time_idx on _hyper_2_10_chunk (never executed)
731+
-> Index Scan Backward using _hyper_2_12_chunk_btest_numeric_time_idx on _hyper_2_12_chunk (actual rows=1.00 loops=1)
732+
-> Index Scan Backward using _hyper_2_11_chunk_btest_numeric_time_idx on _hyper_2_11_chunk (never executed)
683733

684734
:PREFIX SELECT last(quantity, time) FROM btest_numeric;
685735
--- QUERY PLAN ---
@@ -688,24 +738,24 @@ INSERT INTO btest_numeric VALUES('2018-01-20T09:00:43', NULL);
688738
-> Limit (actual rows=1.00 loops=1)
689739
-> Custom Scan (ChunkAppend) on btest_numeric (actual rows=1.00 loops=1)
690740
Order: btest_numeric."time" DESC
691-
-> Index Scan using _hyper_2_10_chunk_btest_numeric_time_idx on _hyper_2_10_chunk (actual rows=1.00 loops=1)
741+
-> Index Scan using _hyper_2_11_chunk_btest_numeric_time_idx on _hyper_2_11_chunk (actual rows=1.00 loops=1)
692742
Index Cond: ("time" IS NOT NULL)
693-
-> Index Scan using _hyper_2_11_chunk_btest_numeric_time_idx on _hyper_2_11_chunk (never executed)
743+
-> Index Scan using _hyper_2_12_chunk_btest_numeric_time_idx on _hyper_2_12_chunk (never executed)
694744
Index Cond: ("time" IS NOT NULL)
695745

696746
:PREFIX SELECT first(time, quantity) FROM btest_numeric;
697747
--- QUERY PLAN ---
698748
Aggregate (actual rows=1.00 loops=1)
699749
-> Append (actual rows=4.00 loops=1)
700-
-> Seq Scan on _hyper_2_10_chunk (actual rows=2.00 loops=1)
701750
-> Seq Scan on _hyper_2_11_chunk (actual rows=2.00 loops=1)
751+
-> Seq Scan on _hyper_2_12_chunk (actual rows=2.00 loops=1)
702752

703753
:PREFIX SELECT last(time, quantity) FROM btest_numeric;
704754
--- QUERY PLAN ---
705755
Aggregate (actual rows=1.00 loops=1)
706756
-> Append (actual rows=4.00 loops=1)
707-
-> Seq Scan on _hyper_2_10_chunk (actual rows=2.00 loops=1)
708757
-> Seq Scan on _hyper_2_11_chunk (actual rows=2.00 loops=1)
758+
-> Seq Scan on _hyper_2_12_chunk (actual rows=2.00 loops=1)
709759

710760
ROLLBACK;
711761
-- we want test results as part of the output too to make sure we produce correct output
@@ -989,6 +1039,40 @@ CREATE INDEX btest_time_alt_idx ON btest(time_alt);
9891039
------
9901040
35.3
9911041

1042+
ROLLBACK;
1043+
-- two FIRST/LAST sharing the value column but ordering on different columns
1044+
BEGIN;
1045+
CREATE TABLE bookend_two_orderings(time timestamptz NOT NULL, time_alt timestamptz NOT NULL, val int NOT NULL);
1046+
SELECT schema_name, table_name, created FROM create_hypertable('bookend_two_orderings', 'time');
1047+
schema_name | table_name | created
1048+
-------------+-----------------------+---------
1049+
public | bookend_two_orderings | t
1050+
1051+
INSERT INTO bookend_two_orderings
1052+
SELECT '2025-01-01'::timestamptz + g * interval '1 minute',
1053+
'2025-01-01'::timestamptz + (201 - g) * interval '1 minute',
1054+
g
1055+
FROM generate_series(1, 200) g;
1056+
CREATE INDEX ON bookend_two_orderings(time);
1057+
CREATE INDEX ON bookend_two_orderings(time_alt);
1058+
:PREFIX SELECT first(val, time), first(val, time_alt) FROM bookend_two_orderings;
1059+
first | first
1060+
-------+-------
1061+
1 | 200
1062+
1063+
:PREFIX SELECT last(val, time), last(val, time_alt) FROM bookend_two_orderings;
1064+
last | last
1065+
------+------
1066+
200 | 1
1067+
1068+
-- DISTINCT sorts on the first/last results, so the optimization must be skipped
1069+
SET enable_hashagg = off;
1070+
:PREFIX SELECT DISTINCT first(val, time), first(val, time_alt) FROM bookend_two_orderings;
1071+
first | first
1072+
-------+-------
1073+
1 | 200
1074+
1075+
RESET enable_hashagg;
9921076
ROLLBACK;
9931077
-- Test with NULL numeric values
9941078
BEGIN;
@@ -1169,6 +1253,24 @@ CREATE INDEX btest_time_alt_idx ON btest(time_alt);
11691253
-- test nested FIRST/LAST in ORDER BY - no optimization possible
11701254
:PREFIX SELECT abs(last(temp, time)) FROM btest ORDER BY abs(last(temp,time));
11711255
ROLLBACK;
1256+
-- two FIRST/LAST sharing the value column but ordering on different columns
1257+
BEGIN;
1258+
CREATE TABLE bookend_two_orderings(time timestamptz NOT NULL, time_alt timestamptz NOT NULL, val int NOT NULL);
1259+
SELECT schema_name, table_name, created FROM create_hypertable('bookend_two_orderings', 'time');
1260+
INSERT INTO bookend_two_orderings
1261+
SELECT '2025-01-01'::timestamptz + g * interval '1 minute',
1262+
'2025-01-01'::timestamptz + (201 - g) * interval '1 minute',
1263+
g
1264+
FROM generate_series(1, 200) g;
1265+
CREATE INDEX ON bookend_two_orderings(time);
1266+
CREATE INDEX ON bookend_two_orderings(time_alt);
1267+
:PREFIX SELECT first(val, time), first(val, time_alt) FROM bookend_two_orderings;
1268+
:PREFIX SELECT last(val, time), last(val, time_alt) FROM bookend_two_orderings;
1269+
-- DISTINCT sorts on the first/last results, so the optimization must be skipped
1270+
SET enable_hashagg = off;
1271+
:PREFIX SELECT DISTINCT first(val, time), first(val, time_alt) FROM bookend_two_orderings;
1272+
RESET enable_hashagg;
1273+
ROLLBACK;
11721274
-- Test with NULL numeric values
11731275
BEGIN;
11741276
TRUNCATE btest_numeric;
@@ -1292,6 +1394,24 @@ CREATE INDEX btest_time_alt_idx ON btest(time_alt);
12921394
-- test nested FIRST/LAST in ORDER BY - no optimization possible
12931395
:PREFIX SELECT abs(last(temp, time)) FROM btest ORDER BY abs(last(temp,time));
12941396
ROLLBACK;
1397+
-- two FIRST/LAST sharing the value column but ordering on different columns
1398+
BEGIN;
1399+
CREATE TABLE bookend_two_orderings(time timestamptz NOT NULL, time_alt timestamptz NOT NULL, val int NOT NULL);
1400+
SELECT schema_name, table_name, created FROM create_hypertable('bookend_two_orderings', 'time');
1401+
INSERT INTO bookend_two_orderings
1402+
SELECT '2025-01-01'::timestamptz + g * interval '1 minute',
1403+
'2025-01-01'::timestamptz + (201 - g) * interval '1 minute',
1404+
g
1405+
FROM generate_series(1, 200) g;
1406+
CREATE INDEX ON bookend_two_orderings(time);
1407+
CREATE INDEX ON bookend_two_orderings(time_alt);
1408+
:PREFIX SELECT first(val, time), first(val, time_alt) FROM bookend_two_orderings;
1409+
:PREFIX SELECT last(val, time), last(val, time_alt) FROM bookend_two_orderings;
1410+
-- DISTINCT sorts on the first/last results, so the optimization must be skipped
1411+
SET enable_hashagg = off;
1412+
:PREFIX SELECT DISTINCT first(val, time), first(val, time_alt) FROM bookend_two_orderings;
1413+
RESET enable_hashagg;
1414+
ROLLBACK;
12951415
-- Test with NULL numeric values
12961416
BEGIN;
12971417
TRUNCATE btest_numeric;

0 commit comments

Comments
 (0)