Skip to content

Commit

Permalink
[#24512] YSQL: Fix incorrect column reference to subplan with OFFSET 0
Browse files Browse the repository at this point in the history
Summary:
There was a bug in our aggregate support implementation. In general, by
the execution time column references in the aggregate functions are
represented as the position number in the underlying plan's target list.
When aggregate is pushed down with the scan, the target list position
is translated to the column number of the scanned relation.

The Postgres optimizer may modify a column reference multiple times, but
in most cases a column reference is initialy represented as the column
number of some relation, as relation infos, including column lists, are
retrieved by the parser. Later on optimizer removes unused columns from
the relation infos, and updates the references accordingly, but the
initial number is stores with the reference. Hence we conveniently used
it to update the reference when we pushdown.

However, there are exceptions. Subqueries are planned independently, and
aggregates on subquery results initially refer the targets of the
subquery, not columns of some table. In most of the cases it does not
create a problem, because we do not push aggregates into subqueries. But
sometimes optimizer can figure out that subquery is trivial and flatten
it, but references' initial numbers are still point to subquery and
therefore update of pushed down column reference is incorrect.

This diff no longer relies on the assumption that the initial column
number stored with the reference is correct. It pulls the referenced
target list entry from the scan and finds there correct column number.
Jira: DB-13426

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressFeature#testPgRegressFeature'

Reviewers: jason, aagrawal, mtakahara

Reviewed By: mtakahara

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D41415
  • Loading branch information
andrei-mart committed Feb 5, 2025
1 parent a5731d6 commit c10148f
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 17 deletions.
36 changes: 23 additions & 13 deletions src/postgres/src/backend/access/yb_access/yb_scan.c
Original file line number Diff line number Diff line change
Expand Up @@ -2809,7 +2809,8 @@ ybcSetupTargets(YbScanDesc yb_scan, YbScanPlan scan_plan, Scan *pg_scan_plan)
* attribute numbers from table-based numbers to index-based ones.
*/
void
YbDmlAppendTargetsAggregate(List *aggrefs, TupleDesc tupdesc, Relation index,
YbDmlAppendTargetsAggregate(List *aggrefs, Scan *outer_plan,
TupleDesc tupdesc, Relation index,
bool xs_want_itup, YbcPgStatement handle)
{
ListCell *lc;
Expand Down Expand Up @@ -2878,18 +2879,27 @@ YbDmlAppendTargetsAggregate(List *aggrefs, TupleDesc tupdesc, Relation index,
}
else if (IsA(tle->expr, Var))
{
Var *var = castNode(Var, tle->expr);
int attno = var->varattno;
/*
* Use original attribute number (varoattno) instead of projected one (varattno)
* as projection is disabled for tuples produced by pushed down operators.
* Change column reference in an aggregate to attribute
* number. Given limited number of cases we support, we
* take a number of assumptions here: the outer plan is a
* plain Scan, and the scan's target list contains only
* simple Vars.
* Support for more generic plan shapes would require
* deep rework of Postgres/PgGate interactions.
*/
int attno = castNode(Var, tle->expr)->varattnosyn;

/*
* For index only scans, translate the table-based
* attribute number to an index-based one.
*/
if (index && xs_want_itup)
attno = YbGetIndexAttnum(index, attno);
if (outer_plan)
{
List *tlist = outer_plan->plan.targetlist;
Assert(var->varno == OUTER_VAR);
Assert(attno > 0);
Assert(attno <= list_length(tlist));
TargetEntry *scan_tle = list_nth_node(TargetEntry, tlist, attno - 1);
Assert(IsA(scan_tle->expr, Var));
attno = castNode(Var, scan_tle->expr)->varattno;
}
Form_pg_attribute attr = TupleDescAttr(tupdesc, attno - 1);
YbcPgTypeAttrs type_attrs = {attr->atttypmod};

Expand Down Expand Up @@ -3112,8 +3122,8 @@ ybcBeginScan(Relation relation,
* non-aggregate and aggregate targets.
*/
if (aggrefs != NIL)
YbDmlAppendTargetsAggregate(aggrefs, ybScan->target_desc, index,
xs_want_itup, ybScan->handle);
YbDmlAppendTargetsAggregate(aggrefs, pg_scan_plan, ybScan->target_desc,
index, xs_want_itup, ybScan->handle);
else
ybcSetupTargets(ybScan, &scan_plan, pg_scan_plan);

Expand Down
1 change: 1 addition & 0 deletions src/postgres/src/backend/access/ybgin/ybginget.c
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,7 @@ ybginDoFirstExec(IndexScanDesc scan, ScanDirection dir)
* IndexOnlyScan, not IndexScan.
*/
YbDmlAppendTargetsAggregate(scan->yb_aggrefs,
NULL,
RelationGetDescr(scan->indexRelation),
scan->indexRelation,
scan->xs_want_itup,
Expand Down
1 change: 1 addition & 0 deletions src/postgres/src/backend/executor/yb_fdw.c
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ ybcSetupScanTargets(ForeignScanState *node)
if (node->yb_fdw_aggrefs != NIL)
{
YbDmlAppendTargetsAggregate(node->yb_fdw_aggrefs,
(Scan *) foreignScan,
RelationGetDescr(ss->ss_currentRelation),
NULL /* index */ ,
false /* xs_want_itup */ ,
Expand Down
6 changes: 3 additions & 3 deletions src/postgres/src/include/access/yb_scan.h
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,9 @@ extern void YbDmlAppendTargetRegular(TupleDesc tupdesc, AttrNumber attnum,
extern void YbDmlAppendTargetRegularAttr(const FormData_pg_attribute *attr,
YbcPgStatement handle);

extern void YbDmlAppendTargetsAggregate(List *aggrefs, TupleDesc tupdesc,
Relation index, bool xs_want_itup,
YbcPgStatement handle);
extern void YbDmlAppendTargetsAggregate(List *aggrefs, Scan *outer_plan,
TupleDesc tupdesc, Relation index,
bool xs_want_itup, YbcPgStatement handle);
extern void YbDmlAppendTargets(List *colrefs, YbcPgStatement handle);

extern void YbAppendPrimaryColumnRef(YbcPgStatement dml, YbcPgExpr colref);
Expand Down
11 changes: 11 additions & 0 deletions src/postgres/src/test/regress/expected/yb_feature_select.out
Original file line number Diff line number Diff line change
Expand Up @@ -321,3 +321,14 @@ SELECT col_smallint Employee_ID,
13 | three
14 | four
(9 rows)

-- GHI #24512
create table test (a bigint, b bigint);
insert into test (a, b) select i, 2024000000000 + i from generate_series(1, 1000) as i;
select max(sub.b) from (select b from test where (b > 2024000000000 AND b < 2025000000000) OFFSET 0 ) as sub;
max
---------------
2024000001000
(1 row)

drop table test;
8 changes: 7 additions & 1 deletion src/postgres/src/test/regress/sql/yb_feature_select.sql
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
-- YB_FEATURE Testsuite: SELECT
-- An introduction on whether or not a feature is supported in YugaByte.
-- This test suite does not go in depth for each command.
--
--
-- SELECT Statements
--
-- Select all.
Expand Down Expand Up @@ -117,3 +117,9 @@ SELECT col_smallint Employee_ID,
FROM feature_tab_dml_identifier
WHERE col_id > 5)
ORDER BY Employee_ID;

-- GHI #24512
create table test (a bigint, b bigint);
insert into test (a, b) select i, 2024000000000 + i from generate_series(1, 1000) as i;
select max(sub.b) from (select b from test where (b > 2024000000000 AND b < 2025000000000) OFFSET 0 ) as sub;
drop table test;

0 comments on commit c10148f

Please sign in to comment.