citusdata / citus

Distributed PostgreSQL as an extension
https://www.citusdata.com
GNU Affero General Public License v3.0
10.63k stars 670 forks source link

PG17 compatibility: Fix Test Failure in local_table_join #7732

Open m3hm3t opened 1 week ago

m3hm3t commented 1 week ago

PostgreSQL 17 seems to have introduced improvements in how correlated subqueries are handled during plan generation. Instead of generating a trivial subplan with WHERE true, it now applies more specific filtering (WHERE (key = 5)), which makes the execution plan more efficient.

https://github.com/postgres/postgres/commit/b262ad44

diff -dU10 -w /__w/citus/citus/src/test/regress/expected/local_table_join.out /__w/citus/citus/src/test/regress/results/local_table_join.out
--- /__w/citus/citus/src/test/regress/expected/local_table_join.out.modified    2024-11-05 09:53:50.423970699 +0000
+++ /__w/citus/citus/src/test/regress/results/local_table_join.out.modified 2024-11-05 09:53:50.463971296 +0000
@@ -1420,32 +1420,32 @@
   ) as subq_1
 ) as subq_2;
 DEBUG:  Wrapping relation "custom_pg_type" to a subquery
 DEBUG:  generating subplan 204_1 for subquery SELECT typdefault FROM local_table_join.custom_pg_type WHERE true
 ERROR:  direct joins between distributed and local tables are not supported
 HINT:  Use CTE's or subqueries to select from local tables and use them in joins
 -- correlated sublinks are not yet supported because of #4470, unless we convert not-correlated table
 SELECT COUNT(*) FROM distributed_table d1 JOIN postgres_table using(key)
 WHERE d1.key IN (SELECT key FROM distributed_table WHERE d1.key = key and key = 5);
 DEBUG:  Wrapping relation "postgres_table" to a subquery
-DEBUG:  generating subplan XXX_1 for subquery SELECT key FROM local_table_join.postgres_table WHERE true
+DEBUG:  generating subplan 206_1 for subquery SELECT key FROM local_table_join.postgres_table WHERE (key OPERATOR(pg_catalog.=) 5)
codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.61%. Comparing base (b29c332) to head (7ba57a6).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## naisila/pg17_support #7732 +/- ## ======================================================== - Coverage 89.61% 89.61% -0.01% ======================================================== Files 274 274 Lines 59689 59689 Branches 7446 7446 ======================================================== - Hits 53490 53488 -2 Misses 4069 4069 - Partials 2130 2132 +2 ```
onurctirtir commented 1 day ago

Hey Mehmet, after your remarks and @onurctirtir's ones, I re-checked this test. You are right that the filter change is not trivial.

However, the filter is not necessary to what the test is about: the test is about correlated sublinks not being yet supported. If we remove "key = 5" from the query, we still have a correlated sublink. So, we can remove "key = 5" from the queries and the result will be the same, and this will get rid of the test difference between versions.

On the other hand, we can add the test with "key = 5" to pg17.sql to see the improvement of Postgres applied in Citus.


So, we can remove "key = 5" from the queries and the result will be the same, and this will get rid of the test difference between versions.

On the other hand, we can add the test with "key = 5" to pg17.sql to see the improvement of Postgres applied in Citus.

All makes sense to me, let's do both.