From 01945bd3a35951107f367a6239dbdde5715f8986 Mon Sep 17 00:00:00 2001 From: Karina <55838532+Green-Chan@users.noreply.github.com> Date: Mon, 8 Jan 2024 19:09:30 +0300 Subject: [PATCH] Fix getting heap tuple size (#7387) This fixes #7230. First of all, using HeapTupleHeaderGetDatumLength(heapTuple) is definetly wrong, it gives a number that's 4 times less than the correct tuple size (heapTuple.t_len). See https://github.com/postgres/postgres/blob/REL_16_0/src/include/access/htup_details.h#L455-L456 https://github.com/postgres/postgres/blob/REL_16_0/src/include/varatt.h#L279 https://github.com/postgres/postgres/blob/REL_16_0/src/include/varatt.h#L225-L226 When I fixed it, the limit_intermediate_size test failed, so I tried to understand what's going on there. In original commit fd546cf these queries were supposed to fail. Then in b3af63c three of the queries that were supposed to fail suddenly worked and tests were changed to pass without understanding why the output had changed or how to keep test testing what it had to test. Even comments saying that these queries should fail were left untouched. Commit message gives no clue about why exactly test has changed: > It seems that when we use adaptive executor instead of task tracker, we > exceed the intermediate result size less in the test. Therefore updated > the tests accordingly. Then 3fda2c3 also blindly raised the limit for one of the queries to keep it working: https://github.com/citusdata/citus/commit/3fda2c325413a3726824ce5f463d60ea84281eb9#diff-a9b7b617f9dfd345318cb8987d5897143ca1b723c87b81049bbadd94dcc86570R19 When in fe3caf3 that HeapTupleHeaderGetDatumLength(heapTuple) call was finally added, one of those test queries became failing again. The other two of them now also failing after the fix. I don't understand how exactly the calculation of "intermediate result size" that is limited by citus.max_intermediate_result_size had changed through b3af63c and fe3caf3, but these numbers are now closer to what they originally were when this limitation was added in fd546cf. So these queries should fail, like in the original version of the limit_intermediate_size test. Co-authored-by: Karina Litskevich (cherry picked from commit 20dc58cf5dfbf7cbb37a134afc26e75f3a79e144) --- .../distributed/executor/tuple_destination.c | 2 +- src/test/regress/citus_tests/run_test.py | 1 + .../expected/limit_intermediate_size.out | 28 ++++++------------- .../regress/sql/limit_intermediate_size.sql | 3 +- 4 files changed, 12 insertions(+), 22 deletions(-) diff --git a/src/backend/distributed/executor/tuple_destination.c b/src/backend/distributed/executor/tuple_destination.c index 3c44d21c0..b3c4b509c 100644 --- a/src/backend/distributed/executor/tuple_destination.c +++ b/src/backend/distributed/executor/tuple_destination.c @@ -109,7 +109,7 @@ TupleStoreTupleDestPutTuple(TupleDestination *self, Task *task, uint64 tupleSize = tupleLibpqSize; if (tupleSize == 0) { - tupleSize = HeapTupleHeaderGetDatumLength(heapTuple); + tupleSize = heapTuple->t_len; } /* diff --git a/src/test/regress/citus_tests/run_test.py b/src/test/regress/citus_tests/run_test.py index 2b1c5210c..0f6153c93 100755 --- a/src/test/regress/citus_tests/run_test.py +++ b/src/test/regress/citus_tests/run_test.py @@ -184,6 +184,7 @@ DEPS = { "foreign_key_to_reference_shard_rebalance": TestDeps( "minimal_schedule", ["remove_coordinator_from_metadata"] ), + "limit_intermediate_size": TestDeps("base_schedule"), } diff --git a/src/test/regress/expected/limit_intermediate_size.out b/src/test/regress/expected/limit_intermediate_size.out index e6fd0e798..996c6536b 100644 --- a/src/test/regress/expected/limit_intermediate_size.out +++ b/src/test/regress/expected/limit_intermediate_size.out @@ -16,7 +16,8 @@ SELECT cte.user_id, cte.value_2 FROM cte,cte2 ORDER BY 1,2 LIMIT 10; ERROR: the intermediate result size exceeds citus.max_intermediate_result_size (currently 2 kB) DETAIL: Citus restricts the size of intermediate results of complex subqueries and CTEs to avoid accidentally pulling large result sets into once place. HINT: To run the current query, set citus.max_intermediate_result_size to a higher value or -1 to disable. -SET citus.max_intermediate_result_size TO 17; +SET citus.max_intermediate_result_size TO 9; +-- regular adaptive executor CTE should fail WITH cte AS MATERIALIZED ( SELECT @@ -38,20 +39,9 @@ FROM ORDER BY 1,2 LIMIT 10; - user_id | value_2 ---------------------------------------------------------------------- - 1 | 0 - 1 | 0 - 1 | 0 - 1 | 0 - 1 | 0 - 1 | 0 - 1 | 0 - 1 | 0 - 1 | 0 - 1 | 0 -(10 rows) - +ERROR: the intermediate result size exceeds citus.max_intermediate_result_size (currently 9 kB) +DETAIL: Citus restricts the size of intermediate results of complex subqueries and CTEs to avoid accidentally pulling large result sets into once place. +HINT: To run the current query, set citus.max_intermediate_result_size to a higher value or -1 to disable. -- router queries should be able to get limitted too SET citus.max_intermediate_result_size TO 2; -- this should pass, since we fetch small portions in each subplan @@ -117,11 +107,9 @@ WITH cte AS MATERIALIZED ( AND EXISTS (select * from cte2, cte3) ) SELECT count(*) FROM cte WHERE EXISTS (select * from cte); - count ---------------------------------------------------------------------- - 105 -(1 row) - +ERROR: the intermediate result size exceeds citus.max_intermediate_result_size (currently 4 kB) +DETAIL: Citus restricts the size of intermediate results of complex subqueries and CTEs to avoid accidentally pulling large result sets into once place. +HINT: To run the current query, set citus.max_intermediate_result_size to a higher value or -1 to disable. SET citus.max_intermediate_result_size TO 3; -- this should fail since the cte-subplan exceeds the limit even if the -- cte2 and cte3 does not diff --git a/src/test/regress/sql/limit_intermediate_size.sql b/src/test/regress/sql/limit_intermediate_size.sql index 8f64c31fd..38ef734e7 100644 --- a/src/test/regress/sql/limit_intermediate_size.sql +++ b/src/test/regress/sql/limit_intermediate_size.sql @@ -17,7 +17,8 @@ cte2 AS MATERIALIZED ( SELECT cte.user_id, cte.value_2 FROM cte,cte2 ORDER BY 1,2 LIMIT 10; -SET citus.max_intermediate_result_size TO 17; +SET citus.max_intermediate_result_size TO 9; +-- regular adaptive executor CTE should fail WITH cte AS MATERIALIZED ( SELECT