Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PG17 compatibility: Fix Test Failure in local_dist_join_mixed #7731

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

m3hm3t
Copy link
Contributor

@m3hm3t m3hm3t commented Nov 11, 2024

PostgreSQL 16 adds an extra condition (id IS NOT NULL) to the subquery. This condition is likely used to ensure that no null values are processed in the subquery. Instead of using the condition id IS NOT NULL, PostgreSQL 17 generates the subplan with a trivial condition (WHERE true), indicating that it does not need to explicitly check for non-null values.

PostgreSQL 17 likely includes optimizations to handle null checks more efficiently. The WHERE (id IS NOT NULL) condition that was present in PostgreSQL 16 may now be considered redundant by the planner, as it is implicitly handled by the query execution engine.

postgres/postgres@b262ad44

 SELECT
        foo1.id
    FROM
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo9,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo8,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo7,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo6,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo5,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo4,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo3,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo2,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo10,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo1
 WHERE
  foo1.id =  foo9.id AND
  foo1.id =  foo8.id AND
  foo1.id =  foo7.id AND
  foo1.id =  foo6.id AND
  foo1.id =  foo5.id AND
  foo1.id =  foo4.id AND
  foo1.id =  foo3.id AND
  foo1.id =  foo2.id AND
  foo1.id =  foo10.id AND
  foo1.id =  foo1.id
ORDER BY 1;
...
-DEBUG:  generating subplan XXX_10 for subquery SELECT id FROM local_dist_join_mixed.local WHERE (id IS NOT NULL)
+DEBUG:  generating subplan XXX_10 for subquery SELECT id FROM local_dist_join_mixed.local WHERE true
...

@m3hm3t m3hm3t self-assigned this Nov 11, 2024
@m3hm3t m3hm3t changed the base branch from main to naisila/pg17_support November 11, 2024 19:03
@m3hm3t m3hm3t changed the title M3hm3t/local dist join mixed Fix Test Failure in local_dist_join_mixed in PG17 Nov 11, 2024
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (release-13.0@a0cd8bd). Learn more about missing BASE report.

Additional details and impacted files
@@               Coverage Diff               @@
##             release-13.0    #7731   +/-   ##
===============================================
  Coverage                ?   89.65%           
===============================================
  Files                   ?      274           
  Lines                   ?    59612           
  Branches                ?     7437           
===============================================
  Hits                    ?    53446           
  Misses                  ?     4034           
  Partials                ?     2132           

@m3hm3t m3hm3t marked this pull request as ready for review November 11, 2024 19:23
@naisila naisila changed the title Fix Test Failure in local_dist_join_mixed in PG17 PG17 compatibility: Fix Test Failure in local_dist_join_mixed Nov 12, 2024
@m3hm3t m3hm3t marked this pull request as draft November 14, 2024 10:01
@naisila naisila force-pushed the naisila/pg17_support branch 4 times, most recently from 46dc966 to 6d036b0 Compare November 20, 2024 11:54
@naisila
Copy link
Member

naisila commented Nov 20, 2024

Here we can simply remove the redundant filter because it wasn't the original intention of this test.

 SELECT
        foo1.id
    FROM
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo9,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo8,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo7,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo6,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo5,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo4,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo3,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo2,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo10,
 (SELECT local.id, local.title FROM local, distributed WHERE local.id = distributed.id ) as foo1
 WHERE
  foo1.id =  foo9.id AND
  foo1.id =  foo8.id AND
  foo1.id =  foo7.id AND
  foo1.id =  foo6.id AND
  foo1.id =  foo5.id AND
  foo1.id =  foo4.id AND
  foo1.id =  foo3.id AND
  foo1.id =  foo2.id AND
  foo1.id =  foo10.id AND
-  foo1.id =  foo1.id
ORDER BY 1;

@naisila naisila force-pushed the naisila/pg17_support branch 6 times, most recently from e108bb8 to c396ce6 Compare November 22, 2024 13:27
@naisila naisila force-pushed the naisila/pg17_support branch from c396ce6 to a12026b Compare December 2, 2024 14:32
@m3hm3t m3hm3t changed the base branch from naisila/pg17_support to m3hm3t/pg17_support December 3, 2024 16:03
@m3hm3t m3hm3t force-pushed the m3hm3t/local_dist_join_mixed branch from 0a8e7ab to 9ac5324 Compare December 3, 2024 16:07
@m3hm3t m3hm3t marked this pull request as ready for review December 3, 2024 17:30
@m3hm3t m3hm3t requested a review from naisila December 3, 2024 17:30
Copy link
Member

@naisila naisila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the filter was originally redundant without any special purpose, we can simply remove the redundant filter, no need for a normalize rule here.
#7731 (comment)

@m3hm3t m3hm3t force-pushed the m3hm3t/pg17_support branch from ed02286 to 383e4a2 Compare December 16, 2024 19:36
Copy link
Member

@naisila naisila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

Reminder to rebase to release-13.0, and only include the changes related to local_dist_join_mixed.sql and local_dist_join_mixed.out

@m3hm3t m3hm3t force-pushed the m3hm3t/local_dist_join_mixed branch 4 times, most recently from 51b75d4 to db513d2 Compare December 19, 2024 18:14
@m3hm3t m3hm3t changed the base branch from m3hm3t/pg17_support to release-13.0 December 19, 2024 18:14
update

update

.

update

comment update

update

tools removal
@m3hm3t m3hm3t force-pushed the m3hm3t/local_dist_join_mixed branch from db513d2 to a72d729 Compare December 19, 2024 18:15
@m3hm3t m3hm3t merged commit acd7b1e into release-13.0 Dec 19, 2024
119 checks passed
@m3hm3t m3hm3t deleted the m3hm3t/local_dist_join_mixed branch December 19, 2024 19:21
naisila added a commit that referenced this pull request Dec 24, 2024
This is the final commit that adds
PG17 compatibility with Citus's current capabilities.

You can use Citus community, release-13.0 branch, with PG17.1.

---------

Specifically, this commit:

- Enables PG17 in the configure script.

- Adds PG17 tests to CI using test images that have 17.1

- Fixes an upgrade test: see below for details
In `citus_prepare_upgrade()`, don't drop any_value when upgrading from
PG16+, because PG16+ has its own any_value function. Attempting to do so
results in the error seen in [pg16-pg17
upgrade](https://github.com/citusdata/citus/actions/runs/11768444117/job/32778340003?pr=7661):
```
ERROR:  cannot drop function any_value(anyelement) because it is required by the database system
CONTEXT:  SQL statement "DROP AGGREGATE IF EXISTS pg_catalog.any_value(anyelement)"
```
When 16 becomes the minimum supported Postgres version, the drop
statements can be removed.

---------

Several PG17 Compatibility commits have been merged before this final one.
All these subtasks are done #7653

See the list below:

Compilation PR: #7699
Ruleutils PR: #7725
Sister PR for tests: citusdata/the-process#159

Helpful smaller PRs:
- #7714
- #7726
- #7731
- #7732
- #7733
- #7738
- #7745
- #7747
- #7748
- #7749
- #7752
- #7755
- #7757
- #7759
- #7760
- #7761
- #7762
- #7765
- #7766
- #7768
- #7769
- #7771
- #7774
- #7776
- #7780
- #7781
- #7785
- #7788
- #7793
- #7796

---------

Co-authored-by: Colm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants