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 multi-1 diffs caused by PG17 optimizer enhancements #7769

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

colm-mchugh
Copy link
Contributor

@colm-mchugh colm-mchugh commented Nov 25, 2024

This fix ensures that the expected DEBUG error messages from the router planner in multi_router_planner, multi_router_planner_fast_path and query_single_shard_table are present with PG17.

In query_single_shard_table the diff:

SELECT COUNT(*) FROM citus_local_table t1
 WHERE t1.b IN (
     SELECT b+1 FROM nullkey_c1_t1 t2 WHERE t2.b = t1.a
 );
-DEBUG:  router planner does not support queries that reference non-colocated distributed tables
+DEBUG:  Local tables cannot be used in distributed queries.

occurred because of this PG17 commit which enables the optimizer to pull up a correlated ANY subquery to a join. The fix inhibits subquery pull up by including a volatile function in the predicate involving the ANY subquery, preserving the pre-PG17 optimizer treatment of the query.

In the case of multi_router_planner and multi_router_planner_fast_path the diffs:

-- partition_column is null clause does not prune out any shards,
 -- all shards remain after shard pruning, not router plannable
 SELECT *
 	FROM articles_hash a
 	WHERE a.author_id is null;
-DEBUG:  Router planner cannot handle multi-shard select queries
+DEBUG:  Creating router plan

are because of this PG17 commit, which enables the optimizer to detect and remove redundant IS (NOT) NULL expressions. The fix is to adjust the table definition so the column used for distribution is not marked NOT NULL, thus preserving the pre-PG17 query planning behavior.

Finallly, the DEBUG logging level is lowered for CREATE MATERIALIZED VIEW AS statements in multi_router_planner and multi_router_planner_fast_path because of this PG17 commit; when creating materialized views, use REFRESH logic to load data, a consequence of which is that with client_min_messages at DEBUG2 Postgres emits extra detail for CREATE MATERIALIZED VIEW AS statements.

CREATE MATERIALIZED VIEW mv_articles_hash_empty AS
 	SELECT * FROM articles_hash WHERE author_id = 1;
 DEBUG:  Creating router plan
 DEBUG:  query has a single distribution column value: 1
+DEBUG:  drop auto-cascades to type multi_router_planner.pg_temp_61391
+DEBUG:  drop auto-cascades to type multi_router_planner.pg_temp_61391[]

Note: the number part can vary. This is because of the REFRESH logic added by the aforementioned commit. Also relevant is this PG17 commit which sets the search_path to 'pg_catalog, pg_temp' during maintenance operations (including REFRESH MATERIALIZED VIEW)

@colm-mchugh colm-mchugh force-pushed the cmchugh/pg17-multi-1-query-adjustments branch from 2d0e9ae to 41b873f Compare November 25, 2024 18:53
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@               Coverage Diff               @@
##             release-13.0    #7769   +/-   ##
===============================================
  Coverage                ?   89.64%           
===============================================
  Files                   ?      274           
  Lines                   ?    59591           
  Branches                ?     7436           
===============================================
  Hits                    ?    53421           
  Misses                  ?     4038           
  Partials                ?     2132           

@colm-mchugh colm-mchugh self-assigned this Nov 25, 2024
@colm-mchugh colm-mchugh force-pushed the cmchugh/pg17-multi-1-query-adjustments branch from 41b873f to 0ec4a68 Compare November 26, 2024 10:32
@colm-mchugh colm-mchugh changed the title PG17 regress sanity: fix multi-1 diffs caused by PG17 optimizer enhancements PG17 compatibility: fix multi-1 diffs caused by PG17 optimizer enhancements Nov 26, 2024
@naisila naisila force-pushed the naisila/pg17_support branch 2 times, most recently from a12026b to 6be0649 Compare December 4, 2024 10:51
@naisila naisila force-pushed the naisila/pg17_support branch from 6be0649 to a8139f9 Compare December 5, 2024 10:18
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.

Really good analysis in the PR description, thanks!

Comment on lines 2003 to 2002
SELECT * FROM articles_hash WHERE author_id = 1;
DEBUG: Distributed planning for a fast-path router query
DEBUG: Creating router plan
DEBUG: query has a single distribution column value: 1
Copy link
Member

Choose a reason for hiding this comment

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

This debug is actually useful to see.

Also, we see the extra pg_temp debugs in 3 tests:
multi_router_planner, multi_mx_router_planner, multi_router_planner_fast_path
I think we qualify for a normalize rule here :) Something like:
/DEBUG: drop auto-cascades to type(.*)pg_temp_(.*)/d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough.. this would delete lines matching the PG17 DEBUG message from all .out files, right?

Thanks for the suggestion! Agreed that its good to preserve the existing DEBUG messages.

Copy link
Member

Choose a reason for hiding this comment

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

this would delete lines matching the PG17 DEBUG message from all .out files

Hopefully yes

@naisila naisila force-pushed the naisila/pg17_support branch 2 times, most recently from c815325 to 0c54b22 Compare December 5, 2024 13:22
@colm-mchugh colm-mchugh force-pushed the cmchugh/pg17-multi-1-query-adjustments branch from 0ec4a68 to f4e9d99 Compare December 5, 2024 15:19
@colm-mchugh colm-mchugh force-pushed the cmchugh/pg17-multi-1-query-adjustments branch from 3d03727 to f051940 Compare December 6, 2024 10:29
@colm-mchugh colm-mchugh changed the base branch from naisila/pg17_support to release-13.0 December 6, 2024 10:40
multi_router_planner_fast_path and query_single_shard_table

Restore the expected DEBUG error messages from the router planner.
In the case of query_single_shard_table the diffs are because of
PG17's ability to pull up correlated ANY subqueries (*). The fix
is to inhibit pull up with a volatile function.

In the case of multi_router_planner and multi_router_planner_fast_path
the diffs are because of PG17's ability to remove redundant IS (NOT)
NULL expressions (**). The fix is to adjust the table definition so
the column used for distribution is not marked NOT NULL.

Finally, add a rule to normalize.sed to ignore additional DEBUG
logging by CREATE MATERIALIZED VIEW (postgres commit b4da732fd64).
This was also impacting regress test multi_mx_router_planner.

(*) https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9f1337639
(**) https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b262ad440
@colm-mchugh colm-mchugh force-pushed the cmchugh/pg17-multi-1-query-adjustments branch from f051940 to 007dab8 Compare December 6, 2024 10:51
@colm-mchugh colm-mchugh merged commit 7c9280f into release-13.0 Dec 6, 2024
119 checks passed
@colm-mchugh colm-mchugh deleted the cmchugh/pg17-multi-1-query-adjustments branch December 6, 2024 11:55
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