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

Adjust max_prepared_transactions only when it is default #7712

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Green-Chan
Copy link
Contributor

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.71%. Comparing base (4b4fa22) to head (84903e7).

❌ Your patch check 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 check has failed because the head coverage (48.71%) 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 (4b4fa22) and HEAD (84903e7). Click for more details.

HEAD has 76 uploads less than BASE
Flag BASE (4b4fa22) HEAD (84903e7)
15_citus_upgrade 1 0
17_regress_check-pytest 1 0
16_regress_check-pytest 1 0
15_16_upgrade 1 0
15_regress_check-pytest 1 0
15_17_upgrade 1 0
16_17_upgrade 1 0
17_regress_check-follower-cluster 1 0
17_regress_check-columnar-isolation 1 0
15_regress_check-columnar-isolation 1 0
16_regress_check-columnar-isolation 1 0
15_regress_check-follower-cluster 1 0
15_regress_check-enterprise-isolation-logicalrep-3 1 0
16_regress_check-enterprise-isolation-logicalrep-2 1 0
17_regress_check-enterprise-failure 1 0
16_regress_check-enterprise-failure 1 0
15_regress_check-columnar 1 0
15_regress_check-enterprise-failure 1 0
17_regress_check-columnar 1 0
16_regress_check-query-generator 1 0
17_regress_check-query-generator 1 0
15_regress_check-enterprise-isolation-logicalrep-2 1 0
17_regress_check-enterprise-isolation-logicalrep-3 1 0
17_regress_check-enterprise-isolation-logicalrep-2 1 0
16_regress_check-enterprise-isolation-logicalrep-3 1 0
16_regress_check-split 1 0
16_regress_check-columnar 1 0
15_arbitrary_configs_3 1 0
17_cdc_installcheck 1 0
17_arbitrary_configs_3 1 0
17_regress_check-operations 1 0
16_regress_check-operations 1 0
15_regress_check-operations 1 0
17_arbitrary_configs_2 1 0
15_arbitrary_configs_2 1 0
17_regress_check-isolation 1 0
15_regress_check-isolation 1 0
16_regress_check-isolation 1 0
16_regress_check-enterprise 1 0
15_regress_check-multi 1 0
17_regress_check-multi 1 0
16_regress_check-multi 1 0
16_arbitrary_configs_2 1 0
16_regress_check-vanilla 1 0
16_regress_check-enterprise-isolation 1 0
15_regress_check-vanilla 1 0
15_regress_check-enterprise 1 0
17_regress_check-enterprise-isolation 1 0
17_regress_check-vanilla 1 0
17_regress_check-multi-mx 1 0
15_regress_check-enterprise-isolation-logicalrep-1 1 0
15_regress_check-enterprise-isolation 1 0
16_regress_check-multi-mx 1 0
16_regress_check-enterprise-isolation-logicalrep-1 1 0
16_regress_check-failure 1 0
15_regress_check-multi-mx 1 0
16_arbitrary_configs_3 1 0
15_regress_check-split 1 0
16_cdc_installcheck 1 0
17_regress_check-failure 1 0
17_regress_check-enterprise-isolation-logicalrep-1 1 0
15_arbitrary_configs_0 1 0
15_regress_check-multi-1 1 0
17_regress_check-multi-1 1 0
17_arbitrary_configs_4 1 0
16_arbitrary_configs_4 1 0
15_arbitrary_configs_4 1 0
17_arbitrary_configs_0 1 0
16_arbitrary_configs_0 1 0
15_arbitrary_configs_1 1 0
16_arbitrary_configs_1 1 0
15_cdc_installcheck 1 0
17_arbitrary_configs_1 1 0
15_regress_check-query-generator 1 0
17_regress_check-enterprise 1 0
17_regress_check-split 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #7712       +/-   ##
===========================================
- Coverage   89.17%   48.71%   -40.47%     
===========================================
  Files         283      283               
  Lines       61023    60469      -554     
  Branches     7617     7464      -153     
===========================================
- Hits        54420    29459    -24961     
- Misses       4415    28375    +23960     
- Partials     2188     2635      +447     
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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
2 participants