Skip to content

Adjust max_prepared_transactions only when it is default #7712

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

Merged
merged 3 commits into from
Apr 24, 2025

Conversation

Green-Chan
Copy link
Contributor

@Green-Chan Green-Chan commented Oct 21, 2024

DESCRIPTION: Adjusts max_prepared_transactions only when it's set to default on PG >= 16

Fixes #7711.
Change AdjustMaxPreparedTransactions to really check if max_prepared_transactions is explicitly set by user, and only adjust max_prepared_transactions when it is default.
This fixes 021_twophase test failure with loaded Citus library after postgres/postgres@b39c5272.

@Green-Chan
Copy link
Contributor Author

Argh, I always forget to check it with older PG versions. find_option is declared static in guc.c, so it won't work. Check using find_options for PG versions >= 16, and keep the old check for older versions is too ugly, isn't it?

*/
#if PG_VERSION_NUM >= PG_VERSION_16
struct config_generic *gconf = find_option("max_prepared_transactions",
Copy link
Member

Choose a reason for hiding this comment

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

Two questions:

  1. Why do we provide "create_placeholders" as "true"?
  2. Should we provide "elevel" as "ERROR" maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comments. I no longer remember why. Right now providing "create_placeholders" as "false" and "elevel" as "ERROR" looks better to me, and it seems to work. Fixed that, rebased to main, and squashed my commits

Copy link

codecov bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 48.72%. Comparing base (bb9d90e) to head (c5d8c40).
Report is 1 commits behind head on main.

❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (48.72%) is below the target coverage (87.50%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (bb9d90e) and HEAD (c5d8c40). Click for more details.

HEAD has 81 uploads less than BASE
Flag BASE (bb9d90e) HEAD (c5d8c40)
15_regress_check-pytest 1 0
15_citus_upgrade 1 0
17_regress_check-pytest 1 0
16_regress_check-pytest 1 0
16_17_upgrade 1 0
15_16_upgrade 1 0
15_17_upgrade 1 0
15_regress_check-columnar-isolation 1 0
16_regress_check-follower-cluster 1 0
17_regress_check-follower-cluster 1 0
17_regress_check-columnar-isolation 1 0
15_regress_check-follower-cluster 1 0
16_regress_check-columnar-isolation 1 0
15_regress_check-vanilla 1 0
15_regress_check-enterprise 1 0
16_regress_check-vanilla 1 0
16_regress_check-split 1 0
17_regress_check-split 1 0
16_regress_check-columnar 1 0
15_regress_check-enterprise-isolation 1 0
15_regress_check-enterprise-isolation-logicalrep-1 1 0
17_regress_check-enterprise 1 0
16_regress_check-enterprise-isolation 1 0
17_regress_check-failure 1 0
16_regress_check-enterprise-isolation-logicalrep-1 1 0
17_regress_check-enterprise-isolation 1 0
17_regress_check-enterprise-isolation-logicalrep-1 1 0
16_regress_check-failure 1 0
16_regress_check-enterprise 1 0
17_regress_check-multi-mx 1 0
15_regress_check-failure 1 0
16_cdc_installcheck 1 0
15_regress_check-multi-mx 1 0
17_cdc_installcheck 1 0
16_arbitrary_configs_3 1 0
15_cdc_installcheck 1 0
16_regress_check-operations 1 0
17_regress_check-operations 1 0
16_arbitrary_configs_5 1 0
15_regress_check-operations 1 0
17_regress_check-vanilla 1 0
17_regress_check-enterprise-isolation-logicalrep-3 1 0
17_regress_check-enterprise-isolation-logicalrep-2 1 0
17_regress_check-columnar 1 0
15_regress_check-columnar 1 0
16_regress_check-enterprise-failure 1 0
16_regress_check-enterprise-isolation-logicalrep-3 1 0
15_regress_check-query-generator 1 0
15_regress_check-enterprise-isolation-logicalrep-2 1 0
17_arbitrary_configs_2 1 0
17_regress_check-isolation 1 0
15_regress_check-isolation 1 0
17_regress_check-multi 1 0
16_arbitrary_configs_4 1 0
16_regress_check-multi 1 0
16_regress_check-query-generator 1 0
15_regress_check-enterprise-failure 1 0
17_arbitrary_configs_0 1 0
17_arbitrary_configs_3 1 0
15_arbitrary_configs_3 1 0
15_regress_check-multi 1 0
16_regress_check-multi-mx 1 0
17_arbitrary_configs_5 1 0
15_arbitrary_configs_5 1 0
16_regress_check-isolation 1 0
16_arbitrary_configs_2 1 0
15_arbitrary_configs_2 1 0
17_arbitrary_configs_4 1 0
15_arbitrary_configs_4 1 0
16_regress_check-multi-1 1 0
17_regress_check-multi-1 1 0
15_regress_check-multi-1 1 0
15_arbitrary_configs_0 1 0
16_arbitrary_configs_0 1 0
15_regress_check-split 1 0
15_regress_check-enterprise-isolation-logicalrep-3 1 0
17_regress_check-query-generator 1 0
15_arbitrary_configs_1 1 0
16_arbitrary_configs_1 1 0
17_regress_check-enterprise-failure 1 0
16_regress_check-enterprise-isolation-logicalrep-2 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #7712       +/-   ##
===========================================
- Coverage   89.20%   48.72%   -40.48%     
===========================================
  Files         283      283               
  Lines       61051    60150      -901     
  Branches     7625     7411      -214     
===========================================
- Hits        54458    29308    -25150     
- Misses       4408    28211    +23803     
- Partials     2185     2631      +446     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@onurctirtir onurctirtir enabled auto-merge (squash) April 8, 2025 13:11
@onurctirtir onurctirtir self-requested a review April 8, 2025 23:22
@onurctirtir onurctirtir merged commit 48d89c9 into citusdata:main Apr 24, 2025
116 of 119 checks passed
@Green-Chan
Copy link
Contributor Author

@onurctirtir , what is your policy on back-porting that kind of changes to released versions? Is it going to get to 13.0 and 12.1? Or it will only get to the future version (13.1, 14.0, or whatever it is)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

021_twophase test failure
3 participants