Skip to content

Commit

Permalink
[BACKPORT 2024.1][#21930] YSQL: Fix Bitmap Scans corrupted results fo…
Browse files Browse the repository at this point in the history
…r rechecked text columns

Summary:
Original commit: 237dad5 / D35399

=== Problem ===
Queries with recheck conditions on text columns had corrupted values:
```lang=sql
create table test_recheck_text(a text);
insert into test_recheck_text VALUES ('i');
create index on test_recheck_text(a ASC);

/*+ BitmapScan(t) Set(yb_enable_bitmapscan true) */
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';
                                    a
----------------------------------------------------------------------------
 \x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F
(1 row)
```

This is because of allocating parts of the tuple into a memory context that gets reset when evaluating recheck conditions.

=== Explanation ===

While implementing Bitmap Scans, I had copied two patterns that do not work well together:

First, from `nodeBitmapHeapscan.c`:
```lang=c
/*
 * If we are using lossy info, we have to recheck the qual
 * conditions at every tuple.
 */
if (tbmres->recheck)
{
	econtext->ecxt_scantuple = slot;
	if (!ExecQualAndReset(node->bitmapqualorig, econtext))
	{
		/* Fails recheck, so drop it and loop back for another */
		InstrCountFiltered2(node, 1);
		ExecClearTuple(slot);
		continue;
	}
}
```

second, from `ybSeqScan.c`:
```lang=c
/* capture all fetch allocations in the short-lived context */
oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
ybFetchNext(ybScan->handle, slot,
            RelationGetRelid(node->ss.ss_currentRelation));
MemoryContextSwitchTo(oldcontext);
```

`nodeYbBitmapTablescan.c` had the following flow:
```
lang=c

oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
ybFetchNext() // load tuple into ecxt_per_tuple_memory
MemoryContextSwitchTo(oldcontext);

...

if (node->recheck_required)
{
	if (!ExecQualAndReset(node->recheck_local_quals, econtext)) // reset ecxt_per_tuple_memory!!
	        ...
}
```
`ybFetchNext` loaded any allocations associated with the tuple into `ecxt_per_tuple_memory`. Then `ExecQualAndReset` cleared it.

These two operations clearly don't work well together, and explain the corrupted value of the query - the allocations associated with the tuple were freed before we could use them.

The solution is to fetch the tuple into a longer-lived memory context OR change `ExecQualAndReset` to `ExecQual`. The first option is more desirable, because we can let the tuples themselves live longer while the intermediate results of `ExecQual` can be freed as soon as possible.
Jira: DB-10845

Test Plan:
```
./yb_build.sh  --java-test 'org.yb.pgsql.TestPgRegressYbBitmapScans'
```

Reviewers: tnayak, amartsinchyk, mtakahara

Reviewed By: tnayak

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D35631
  • Loading branch information
timothy-e committed Jun 11, 2024
1 parent 8ab2c1e commit 41e4aae
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 4 deletions.
4 changes: 0 additions & 4 deletions src/postgres/src/backend/executor/nodeYbBitmapTablescan.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ YbBitmapTableNext(YbBitmapTableScanState *node)
YbTBMIterateResult *ybtbmres;
HeapScanDesc scandesc;
ExprContext *econtext;
MemoryContext oldcontext;
YbScanDesc ybScan;

/*
Expand Down Expand Up @@ -131,11 +130,8 @@ YbBitmapTableNext(YbBitmapTableScanState *node)
/* We have yb_fetch_row_limit rows fetched, get them one by one */
while (true)
{
/* capture all fetch allocations in the short-lived context */
oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
ybFetchNext(ybScan->handle, slot,
RelationGetRelid(node->ss.ss_currentRelation));
MemoryContextSwitchTo(oldcontext);

if (ybtbmres)
++ybtbmres->index;
Expand Down
27 changes: 27 additions & 0 deletions src/postgres/src/test/regress/expected/yb_bitmap_scans.out
Original file line number Diff line number Diff line change
Expand Up @@ -1056,6 +1056,33 @@ INSERT INTO test_false VALUES (1, 1), (2, 2);
Storage Index Read Requests: 1
(12 rows)

--
-- #21930: test recheck conditions on text columns
--
CREATE TABLE test_recheck_text(a TEXT);
INSERT INTO test_recheck_text(a) VALUES ('i');
CREATE INDEX ON test_recheck_text(a ASC);
/*+ BitmapScan(t) */ EXPLAIN (ANALYZE, DIST, COSTS OFF, SUMMARY OFF)
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';
QUERY PLAN
----------------------------------------------------------------------------
YB Bitmap Table Scan on test_recheck_text t (actual rows=1 loops=1)
Storage Recheck Cond: ((a < 'j'::text) AND (a = 'i'::text))
Storage Table Read Requests: 1
Storage Table Rows Scanned: 1
-> Bitmap Index Scan on test_recheck_text_a_idx (actual rows=1 loops=1)
Index Cond: ((a < 'j'::text) AND (a = 'i'::text))
Storage Index Read Requests: 1
Storage Index Rows Scanned: 1
(8 rows)

/*+ BitmapScan(t) */
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';
a
---
i
(1 row)

--
-- test recheck index conditions
--
Expand Down
28 changes: 28 additions & 0 deletions src/postgres/src/test/regress/expected/yb_bitmap_scans_colo.out
Original file line number Diff line number Diff line change
Expand Up @@ -1086,6 +1086,34 @@ INSERT INTO test_false VALUES (1, 1), (2, 2);
Storage Index Read Requests: 1
(13 rows)

--
-- #21930: test recheck conditions on text columns
--
CREATE TABLE test_recheck_text(a TEXT);
INSERT INTO test_recheck_text(a) VALUES ('i');
CREATE INDEX ON test_recheck_text(a ASC);
/*+ BitmapScan(t) */ EXPLAIN (ANALYZE, DIST, COSTS OFF, SUMMARY OFF)
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';
QUERY PLAN
----------------------------------------------------------------------------
YB Bitmap Table Scan on test_recheck_text t (actual rows=1 loops=1)
Storage Recheck Cond: ((a < 'j'::text) AND (a = 'i'::text))
Storage Table Read Requests: 1
Storage Table Rows Scanned: 1
-> Bitmap Index Scan on test_recheck_text_a_idx (actual rows=1 loops=1)
Index Cond: ((a < 'j'::text) AND (a = 'i'::text))
Storage Table Rows Scanned: 1
Storage Index Read Requests: 1
Storage Index Rows Scanned: 1
(9 rows)

/*+ BitmapScan(t) */
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';
a
---
i
(1 row)

--
-- test recheck index conditions
--
Expand Down
12 changes: 12 additions & 0 deletions src/postgres/src/test/regress/sql/yb_bitmap_scans.sql
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,18 @@ INSERT INTO test_false VALUES (1, 1), (2, 2);
/*+ BitmapScan(test_false) */ EXPLAIN (ANALYZE, DIST, COSTS OFF, SUMMARY OFF) SELECT * FROM test_false WHERE (a <= 1 AND a = 2);
/*+ BitmapScan(test_false) */ EXPLAIN (ANALYZE, DIST, COSTS OFF, SUMMARY OFF) SELECT * FROM test_false WHERE (a = 1 AND a = 2) OR b = 0;

--
-- #21930: test recheck conditions on text columns
--
CREATE TABLE test_recheck_text(a TEXT);
INSERT INTO test_recheck_text(a) VALUES ('i');
CREATE INDEX ON test_recheck_text(a ASC);

/*+ BitmapScan(t) */ EXPLAIN (ANALYZE, DIST, COSTS OFF, SUMMARY OFF)
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';
/*+ BitmapScan(t) */
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';

--
-- test recheck index conditions
--
Expand Down
12 changes: 12 additions & 0 deletions src/postgres/src/test/regress/sql/yb_bitmap_scans_colo.sql
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,18 @@ INSERT INTO test_false VALUES (1, 1), (2, 2);
/*+ BitmapScan(test_false) */ EXPLAIN (ANALYZE, DIST, COSTS OFF, SUMMARY OFF) SELECT * FROM test_false WHERE (a <= 1 AND a = 2);
/*+ BitmapScan(test_false) */ EXPLAIN (ANALYZE, DIST, COSTS OFF, SUMMARY OFF) SELECT * FROM test_false WHERE (a = 1 AND a = 2) OR b = 0;

--
-- #21930: test recheck conditions on text columns
--
CREATE TABLE test_recheck_text(a TEXT);
INSERT INTO test_recheck_text(a) VALUES ('i');
CREATE INDEX ON test_recheck_text(a ASC);

/*+ BitmapScan(t) */ EXPLAIN (ANALYZE, DIST, COSTS OFF, SUMMARY OFF)
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';
/*+ BitmapScan(t) */
SELECT * FROM test_recheck_text AS t WHERE a = 'i' AND a < 'j';

--
-- test recheck index conditions
--
Expand Down

0 comments on commit 41e4aae

Please sign in to comment.