PG17 compatibility: Resolve compilation issues #7699

naisila · 2024-10-06T19:36:14Z

PG17 compatibility - Part 1

This PR, together with #7700, provides successful compilation against PG17.0.

Notes to reviewer:

Review commit by commit - it's much cleaner that way.
When merged, Enable configure and add pg17 build test commits will be reverted. They are part of the PR to make sure that the PG17 build is actually successful - see Build for PG17 in the PR checks. These changes will be merged to the release-13.0 branch later, when the PG17 support is fully added.

(Original PR against the main branch #7651)

codecov · 2024-10-06T19:43:00Z

Codecov Report

Attention: Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.

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

Additional details and impacted files

@@               Coverage Diff               @@
##             release-13.0    #7699   +/-   ##
===============================================
  Coverage                ?   89.65%           
===============================================
  Files                   ?      274           
  Lines                   ?    59584           
  Branches                ?     7436           
===============================================
  Hits                    ?    53418           
  Misses                  ?     4035           
  Partials                ?     2131

jmealo · 2024-10-08T18:57:51Z

I'm seeing a different issue compiling this branch on arm64/MacOS than I do on main against 16 (neither work, so this isn't a regression):

Against 17 (this branch):

Undefined symbols for architecture arm64:
  "_deparse_shard_query", referenced from:
      _GenerateTaskListWithColocatedIntermediateResults in repartition_executor.o
      _GenerateTaskListWithRedistributedResults in repartition_executor.o
      _DeparseTaskQuery in deparse_shard_query.o
      _CreateInsertSelectPlan in insert_select_planner.o
      _CacheLocalPlanForShardQuery in local_plan_cache.o
      _deparse_shard_query_test in deparse_shard_query.o
  "_generate_operator_name", referenced from:
      _GetFunctionDDLCommand in function.o
  "_generate_relation_name", referenced from:
      _PreprocessGrantStmt in grant.o
      _PreprocessAlterPolicyStmt in policy.o
      _pg_get_tableschemadef_string in citus_ruleutils.o
      _pg_get_tablecolumnoptionsdef_string in citus_ruleutils.o
      _pg_get_table_grants in citus_ruleutils.o
  "_getOwnedSequences_internal", referenced from:
      _ConvertTable in alter_table.o
      _ExtractDefaultColumnsAndOwnedSequences in sequence.o
  "_get_merged_argument_list", referenced from:
      _CallDistributedProcedureRemotely in call.o
  "_pg_get_query_def", referenced from:
      _BuildSelectStatementViaStdPlanner in combine_query_planner.o
      _DeparseTaskQuery in deparse_shard_query.o
      _CacheLocalPlanForShardQuery in local_plan_cache.o
      _CreatePhysicalDistributedPlan in multi_physical_planner.o
      _QueryPushdownSqlTaskList in multi_physical_planner.o
      _GenerateSubplansForSubqueriesAndCTEs in recursive_planning.o
      _RecursivelyPlanSubqueriesAndCTEs in recursive_planning.o
      ...
  "_pg_get_rule_expr", referenced from:
      _CallDistributedProcedureRemotely in call.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [citus.dylib] Error 1
make: *** [extension] Error 2

Main against 16

ld: warning: ignoring file '/opt/local/lib/libcurl.4.dylib': found architecture 'arm64', required architecture 'x86_64'
Undefined symbols for architecture x86_64:
  "_curl_easy_cleanup", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_easy_getinfo", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_easy_init", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_easy_perform", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_easy_setopt", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
      _CollectBasicUsageStatistics in statistics_collection.o
      _CollectBasicUsageStatistics in statistics_collection.o
      _CollectBasicUsageStatistics in statistics_collection.o
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_easy_strerror", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_global_cleanup", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_global_init", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_slist_append", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
      _CollectBasicUsageStatistics in statistics_collection.o
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_slist_free_all", referenced from:
      _CollectBasicUsageStatistics in statistics_collection.o
  "_curl_version_info", referenced from:
      _WarnIfSyncDNS in statistics_collection.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [citus.dylib] Error 1
make: *** [extension] Error 2

onurctirtir · 2024-10-15T08:01:46Z

src/backend/distributed/planner/multi_explain.c

+		if (es->memory)
+		{
+			MemoryContextSwitchTo(saved_ctx);
+			MemoryContextMemConsumed(planner_ctx, &mem_counters);


should we switch back to older context after L454

I followed the logic in the Postgres code here. Let me paste the relevant lines, where the context is not switched back. Do you think that is enough?
https://github.com/postgres/postgres/blob/REL_17_0/src/backend/commands/explain.c#L495-L512
https://github.com/postgres/postgres/blob/REL_17_0/src/backend/commands/prepare.c#L637-L642

onurctirtir · 2024-10-15T08:25:46Z

Note to self; Only remaining commit to review is cfba6ff.

update: done

onurctirtir · 2024-10-15T08:26:21Z

Btw, thank you for cleanly separating the work into commits, it made reviews so much easier!

onurctirtir

LGTM. @rajeshkt78, would you like to review cfba6ff since it introduces some changes to cdc subsystem, or should we just merge this?

src/backend/distributed/deparser/citus_ruleutils.c

src/include/pg_version_compat.h

emelsimsek · 2024-10-15T10:43:11Z

Looks like PG 17 introduced a new ACL called ACL_MAINTAIN

PG commit:
ecb0fd33720fab91df1207e85704f382f55e1eb7
postgres/postgres@ecb0fd3

Our deparser does not handle that

citus/src/backend/distributed/deparser/citus_ruleutils.c

Line 1351 in f695971

elog(ERROR, "unrecognized aclright: %d", aclright);

I understand that this commit is for getting Citus compile against PG17.

Let's create an issue to track "ACL_MAINTAIN" should be supported.

src/include/pg_version_compat.h

emelsimsek · 2024-10-15T11:17:48Z

MergeAction can have 3 merge kinds (now enum) in PG17, write compat
Relevant PG commit:
0294df2f1f842dfb0eed79007b21016f486a3c6c
postgres/postgres@0294df2

New merge kinds should be handled in deparser and Merge logic in Citus.
- MATCHED BY SOURCE
- MATCHED_BY_TARGET

Let's track this as a github issue.

src/backend/distributed/planner/multi_explain.c

src/backend/distributed/planner/multi_physical_planner.c

emelsimsek · 2024-10-15T12:47:51Z

src/include/distributed/citus_nodes.h

-)
-
-#endif
+static inline CitusNode *


Should we delete two lines here:

citus/src/backend/distributed/utils/citus_nodefuncs.c

Line 56 in f695971

/* support for CitusNewNode() macro */

We still have CitusNewNode(size_t size, CitusNodeTag tag) function, so I guess not.

CitusNode *newCitusNodeMacroHolder; is not referenced anywhere. So I assumed it was related to getting around an issue with macro usage. Now since we have a function instead, this line may be redundant and confusing.

Just noticed that, you are right. It is redundant. Let me remove it.

.github/workflows/build_and_test.yml

This is prep work for successful compilation with PG17 PG17added foreach_ptr, foreach_int and foreach_oid macros Relevant PG commit 14dd0f27d7cd56ffae9ecdbe324965073d01a9ff postgres/postgres@14dd0f2 We already have these macros, but they are different with the PG17 ones because our macros take a DECLARED variable, whereas the PG16 macros declare a locally-scoped loop variable themselves. Hence I am renaming our macros to foreach_declared_ I am separating this into its own PR since it touches many files. The main compilation PR is #7699

Relevant PG commit d060e921ea5aa47b6265174c32e1128cebdbc3df postgres/postgres@d060e92

Relevant PG commit 041b96802efa33d2bc9456f2ad946976b92b5ae1 postgres/postgres@041b968

Relevant PG commit: 89e5ef7e21812916c9cf9fcf56e45f0f74034656 postgres/postgres@89e5ef7

Relevant PG commit: 08e6344fd6423210b339e92c069bb979ba4e7cd6 postgres/postgres@08e6344

Relevant PG commit: f696c0cd5f299f1b51e214efc55a22a782cc175d postgres/postgres@f696c0c

Relevant PG commit: de3600452b61d1bc3967e9e37e86db8956c8f577 postgres/postgres@de36004

Relevant PG commit: 50c67c2019ab9ade8aa8768bfe604cd802fe8591 postgres/postgres@50c67c2

Relevant PG commit: 509199587df73f06eda898ae13284292f4ae573a postgres/postgres@5091995

Relevant PG commit: 75680c3d805e2323cd437ac567f0677fdfc7b680 postgres/postgres@75680c3

Relevant PG commit: 0294df2f1f842dfb0eed79007b21016f486a3c6c postgres/postgres@0294df2

Relevant PG commit: 5de890e3610d5a12cdaea36413d967cf5c544e20 postgres/postgres@5de890e

…ON_COUNT Relevant PG commit: a6be0600ac3b71dda8277ab0fcbe59ee101ac1ce postgres/postgres@a6be060

Relevant PG commit: 9e9931d2bf40e2fea447d779c2e133c2c1256ef3 postgres/postgres@9e9931d

Relevant PG commits: 28f3915b73f75bd1b50ba070f56b34241fe53fd1 postgres/postgres@28f3915 ab355e3a88de745607f6dd4c21f0119b5c68f2ad postgres/postgres@ab355e3 024c521117579a6d356050ad3d78fdc95e44eefa postgres/postgres@024c521

postgres refactored newNode() in PG 17, the main point for doing this is the original tricks is no longer neccessary for modern compilers[1]. This does the same for Citus. This should have no backward compatibility issues since it just replaces palloc0fast with palloc0. This is good for forward compatibility since palloc0fast no longer exists in PG 17. [1] https://www.postgresql.org/message-id/[email protected] (cherry picked from commit 4b295cc)

…ctions for it

…ve been renamed

naisila · 2024-10-16T19:05:20Z

Looks like PG 17 introduced a new ACL called ACL_MAINTAIN
...
I understand that this commit is for getting Citus compile against PG17.

MergeAction can have 3 merge kinds (now enum) in PG17, write compat

@emelsimsek you are right, it's not part of the compile PR. We might need a separate issue for new features in PG17 which will encapsulate these subtasks, like this one: #7138. For now, I added your comments in here #7708

naisila · 2024-10-16T19:58:49Z

@jmealo Thanks for catching that, as it's not a regression caused by this PR I opened an issue to track this: #7707

This reverts commit fb4efeb.

This reverts commit 7497419.

PG17 compatibility - Part 2 #7699 was the first PG17 compatibility PR merged to main branch, which provided ONLY successful Citus compilation with PG17.0. This PR, consider it as Part 2, provides ruleutils changes for PG17. Ruleutils changes is the first thing we should merge, after successful build. It's the core for deparsing logic in Citus. # Question: How do we add ruleutils changes? - We add a new ruleutils file specific to PG17. - We keep track of the changes in Postgres's ruleutils file from here https://github.com/postgres/postgres/commits/REL_17_0/src/backend/utils/adt/ruleutils.c - Per each commit in that history that belongs only to 17.0, we add the relevant changes to static functions to our ruleutils file for PG17. It's like a manual commit copying. # Check the PR's commits for detailed steps https://github.com/citusdata/citus/pull/7725/commits

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

naisila force-pushed the naisila/pg17_compile branch from a53a90f to 65d055e Compare October 6, 2024 19:38

naisila force-pushed the naisila/pg17_compile branch from 65d055e to e71d521 Compare October 6, 2024 19:46

naisila changed the base branch from release-13.0 to naisila/foreach_macros October 6, 2024 19:46

naisila mentioned this pull request Oct 6, 2024

Rename foreach_ macros to foreach_declared_ macros #7700

Merged

jmealo mentioned this pull request Oct 8, 2024

PostgreSQL 17 support #7605

Open

onurctirtir reviewed Oct 15, 2024

View reviewed changes

onurctirtir self-requested a review October 15, 2024 08:32

onurctirtir approved these changes Oct 15, 2024

View reviewed changes

emelsimsek requested changes Oct 15, 2024

View reviewed changes

src/backend/distributed/deparser/citus_ruleutils.c Outdated Show resolved Hide resolved

src/backend/distributed/deparser/citus_ruleutils.c Outdated Show resolved Hide resolved

emelsimsek reviewed Oct 15, 2024

View reviewed changes

src/include/pg_version_compat.h Show resolved Hide resolved

emelsimsek requested changes Oct 15, 2024

View reviewed changes

src/include/pg_version_compat.h Outdated Show resolved Hide resolved

emelsimsek reviewed Oct 15, 2024

View reviewed changes

src/backend/distributed/planner/multi_explain.c Show resolved Hide resolved

emelsimsek reviewed Oct 15, 2024

View reviewed changes

src/backend/distributed/planner/multi_physical_planner.c Show resolved Hide resolved

emelsimsek reviewed Oct 15, 2024

View reviewed changes

.github/workflows/build_and_test.yml Outdated Show resolved Hide resolved

Base automatically changed from naisila/foreach_macros to release-13.0 October 16, 2024 14:01

naisila added 7 commits October 16, 2024 21:28

Enable configure

fb4efeb

Remove ExecFreeExprContext call

4685be7

Relevant PG commit d060e921ea5aa47b6265174c32e1128cebdbc3df postgres/postgres@d060e92

PG17 uses streaming IO in analyze, fix scan_analyze_next_block function

b4d36f6

Relevant PG commit 041b96802efa33d2bc9456f2ad946976b92b5ae1 postgres/postgres@041b968

Define ObjectClass for PG17+ only since it's removed

56f85a1

Relevant PG commit: 89e5ef7e21812916c9cf9fcf56e45f0f74034656 postgres/postgres@89e5ef7

Remove ReorderBufferTupleBuf structure.

f5c3b0a

Relevant PG commit: 08e6344fd6423210b339e92c069bb979ba4e7cd6 postgres/postgres@08e6344

Define colliculocale and daticulocale since they have been renamed

ad8eaf7

Relevant PG commit: f696c0cd5f299f1b51e214efc55a22a782cc175d postgres/postgres@f696c0c

makeStringConst defined in PG17

b81c8c8

Relevant PG commit: de3600452b61d1bc3967e9e37e86db8956c8f577 postgres/postgres@de36004

naisila and others added 12 commits October 16, 2024 21:28

Use ResourceOwner to track WaitEventSet in PG17

7ac4794

Relevant PG commit: 50c67c2019ab9ade8aa8768bfe604cd802fe8591 postgres/postgres@50c67c2

getIdentitySequence now uses Relation instead of relation_id

55f7671

Relevant PG commit: 509199587df73f06eda898ae13284292f4ae573a postgres/postgres@5091995

Remove no-op tuplestore_donestoring function

363c8aa

Relevant PG commit: 75680c3d805e2323cd437ac567f0677fdfc7b680 postgres/postgres@75680c3

MergeAction can have 3 merge kinds (now enum) in PG17, write compat

0694250

Relevant PG commit: 0294df2f1f842dfb0eed79007b21016f486a3c6c postgres/postgres@0294df2

EXPLAIN (MEMORY) is added, make changes to ExplainOnePlan

5446088

Relevant PG commit: 5de890e3610d5a12cdaea36413d967cf5c544e20 postgres/postgres@5de890e

LIMIT_OPTION_DEFAULT has been removed as it's useless, use LIMIT_OPTI…

6bffd8a

…ON_COUNT Relevant PG commit: a6be0600ac3b71dda8277ab0fcbe59ee101ac1ce postgres/postgres@a6be060

write compat for create_foreignscan_path bcs of more arguments in PG17

c23c61d

Relevant PG commit: 9e9931d2bf40e2fea447d779c2e133c2c1256ef3 postgres/postgres@9e9931d

add pg17 build test

7497419

Address review on attstattarget is nullable, define pg compatible fun…

ef60a25

…ctions for it

Address review on Define colliculocale and daticulocale since they ha…

06727b6

…ve been renamed

naisila mentioned this pull request Oct 16, 2024

PG17.1 Support - Regression tests sanity #7653

Closed

45 tasks

Address review use int instead of int16 and int32

cd76567

naisila force-pushed the naisila/pg17_compile branch from e71d521 to cd76567 Compare October 16, 2024 19:54

naisila mentioned this pull request Oct 16, 2024

Undefined symbols for architecture arm64 when compiling #7707

Open

naisila requested a review from emelsimsek October 16, 2024 19:59

naisila mentioned this pull request Oct 16, 2024

PG17 compatibility: Resolve compilation issues #7651

Closed

emelsimsek approved these changes Oct 17, 2024

View reviewed changes

naisila added 3 commits October 17, 2024 14:29

Address reviews

980a425

Revert "Enable configure"

532cc95

This reverts commit fb4efeb.

Revert "add pg17 build test"

c3e0bc8

This reverts commit 7497419.

naisila merged commit da2624c into release-13.0 Oct 17, 2024
120 checks passed

naisila deleted the naisila/pg17_compile branch October 17, 2024 12:37

This was referenced Nov 10, 2024

PG17 compatibility: ruleutils #7725

Merged

Adds PG17.1 support - Regression tests sanity #7661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PG17 compatibility: Resolve compilation issues #7699

PG17 compatibility: Resolve compilation issues #7699

naisila commented Oct 6, 2024 •

edited

Loading

codecov bot commented Oct 6, 2024 •

edited

Loading

jmealo commented Oct 8, 2024 •

edited

Loading

onurctirtir Oct 15, 2024

naisila Oct 16, 2024

onurctirtir commented Oct 15, 2024 •

edited

Loading

onurctirtir commented Oct 15, 2024

onurctirtir left a comment

emelsimsek commented Oct 15, 2024

emelsimsek commented Oct 15, 2024

emelsimsek Oct 15, 2024

naisila Oct 16, 2024 •

edited

Loading

emelsimsek Oct 17, 2024

naisila Oct 17, 2024

naisila commented Oct 16, 2024 •

edited

Loading

naisila commented Oct 16, 2024

PG17 compatibility: Resolve compilation issues #7699

PG17 compatibility: Resolve compilation issues #7699

Conversation

naisila commented Oct 6, 2024 • edited Loading

codecov bot commented Oct 6, 2024 • edited Loading

Codecov Report

jmealo commented Oct 8, 2024 • edited Loading

onurctirtir Oct 15, 2024

Choose a reason for hiding this comment

naisila Oct 16, 2024

Choose a reason for hiding this comment

onurctirtir commented Oct 15, 2024 • edited Loading

onurctirtir commented Oct 15, 2024

onurctirtir left a comment

Choose a reason for hiding this comment

emelsimsek commented Oct 15, 2024

emelsimsek commented Oct 15, 2024

emelsimsek Oct 15, 2024

Choose a reason for hiding this comment

naisila Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

emelsimsek Oct 17, 2024

Choose a reason for hiding this comment

naisila Oct 17, 2024

Choose a reason for hiding this comment

naisila commented Oct 16, 2024 • edited Loading

naisila commented Oct 16, 2024

naisila commented Oct 6, 2024 •

edited

Loading

codecov bot commented Oct 6, 2024 •

edited

Loading

jmealo commented Oct 8, 2024 •

edited

Loading

onurctirtir commented Oct 15, 2024 •

edited

Loading

naisila Oct 16, 2024 •

edited

Loading

naisila commented Oct 16, 2024 •

edited

Loading