PG17 compatibility: Fix Test Failure in local_table_join

[m3hm3t]

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.

postgres/postgres@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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@                   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     

[naisila]

We have just two lines with the same change (key OPERATOR(pg_catalog.=) 5)
How about a normalized line, we can change the table names such that the normalization rule detects the line easily.
e.g.
postgres_table_key5_filtering
distributed_table_key5_filtering

Just an idea, could be better. But I am reluctant of merging a new file with 1659 new lines just for 2 lines change in the test output. What do you think?

[m3hm3t]

Applied the solution

[naisila]

Thanks, these changes make sense. Based on your solution, I think we can leverage renaming the table even further by using a table alias, this will make changes even more minimal. What I mean by this:
In order to include everything with one normalize line, and avoid creating a new table, we can make use of aliases. For example, table_diff_filtering alias. We can include this alias in local_dist_join_mixed line difference as well. And we can change the output to WHERE true
(in local_table_join, WHERE true is in PG16; in local_dist_join_mixed, WHERE true is in PG17, but we don't need to document this part)

I did a sketch commit and it seems to work. https://github.com/citusdata/citus/tree/naisila/try_filtering_diff
Could you please update this PR accordingly?

(PS: We had used this table alias trick in PG16 support as well, I believe it deserves to be added to the documentation.)

[naisila]

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.

[onurctirtir]

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.