citusdata / citus

Distributed PostgreSQL as an extension
https://www.citusdata.com
GNU Affero General Public License v3.0
10.63k stars 670 forks source link

021_twophase test failure #7711

Open Green-Chan opened 1 month ago

Green-Chan commented 1 month ago

In commit postgres/postgres@b39c5272 021_twophase test was changed, and it doesn't pass with loaded Citus library any more. Apparently, when max_prepared_transactions is set to zero, Citus overrides it, while the test sets this GUC to zero and relies on this.

Here is how I run this test. In postgres build directory:

echo "shared_preload_libraries = 'citus'" > temp.conf
TEMP_CONFIG=$PWD/temp.conf make installcheck -C src/test/subscription

Citus overrides max_prepared_transactions in function AdjustMaxPreparedTransactions:

/*
 * As Citus uses 2PC internally, there always should be some available. As
 * the default is 0, we increase it to something appropriate
 * (connections * 2 currently).  If the user explicitly configured 2PC, we
 * leave the configuration alone - there might have been intent behind the
 * decision.
 */
if (max_prepared_xacts == 0)
{
    ...
}

The comment is slightly inconsistent with the code here. If max_prepared_transactions is set to zero explicitly (as it is in 021_twophase test), Citus overrides it anyway.

Do you think fixing Citus to pass this test worth it? I'm going to create a pull request soon, where I change AdjustMaxPreparedTransactions to really check if max_prepared_transactions is explicitly set by user. Please take a look.

I also believe that it should be mentioned in the documentation that Citus overrides max_prepared_transactions in some cases.