Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility.

On Thu, Jan 16, 2025 at 4:53 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Jan 15, 2025 at 9:24 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> >
> >
> > On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna <khannashubham1197(at)gmail(dot)com> wrote:
> > > Previously, the warning was necessary because the 'two-phase' option
> > > was not available, and users needed to be informed about the default
> > > behavior regarding 'two-phase' commit. However, with the recent
> > > addition of the 'two-phase' option, users can now directly configure
> > > this behavior during the setup process.
> > > Given this enhancement, the warning message is no longer relevant and
> > > should be removed from the documentation to reflect the latest changes
> > > accurately. Updating the documentation will help ensure that it aligns
> > > with the current functionality and avoids any potential confusion for
> > > users.
> >
> > Hi Shubham,
> >
> > Even though the documentation is updated, the actual code still gives a warning, when you try and create pg_createsubscriber with the --enable-two-phase option:
> >
> > pg_createsubscriber: warning: two_phase option will not be enabled for replication slots
> > pg_createsubscriber: detail: Subscriptions will be created with the two_phase option disabled. Prepared transactions will be replicated at COMMIT PREPARED.
> >
> > This is coming from code in check_publisher()
> >
> > if (max_prepared_transactions != 0)
> > {
> > pg_log_warning("two_phase option will not be enabled for replication slots");
> > pg_log_warning_detail("Subscriptions will be created with the two_phase option disabled. "
> > "Prepared transactions will be replicated at COMMIT PREPARED.");
> > }
> >
> > I think this code needs to be updated as well.
> >
>
> Hi Shubham.
>
> I'm not so sure about the documentation change of v10.
>
> Sure, we can remove the warnings in the docs and just assume/expect
> the user to be aware that there is a new '--enable-two-phase' option
> that they should be using. That is what v10 is now doing.
>
> OTOH, if the user (accidentally?) omits the new '--enable-two-phase'
> switch then all this two-phase documentation still seems relevant.
>
> ~
>
> In other words, we could've left all this documented information
> intact, but just qualify the paragraph. For example:
>
> CURRENTLY (v9)
> pg_createsubscriber sets up logical replication with two-phase commit
> disabled. This means that any prepared transactions will be replicated
> at the time of COMMIT PREPARED, without advance preparation. Once
> setup is complete, you can manually drop and re-create the
> subscription(s) with the two_phase option enabled.
>
>
> SUGGESTION
> Unless the '--enable-two-phase' switch is specified,
> pg_createsubscriber sets up ...
>
> ~
>
> Similarly, to help (accident prone?) users we could leave this code
> warning [1] intact, but just change the condition slightly, and add a
> helpful hint on how to avoid the problem.
>
> CURRENTLY (v10)
> if (max_prepared_transactions != 0)
> {
> pg_log_warning("two_phase option will not be enabled for
> replication slots");
> pg_log_warning_detail("Subscriptions will be created with the
> two_phase option disabled. "
> "Prepared transactions will be
> replicated at COMMIT PREPARED.");
> }
>
> SUGGESTION
> if (max_prepared_transactions != 0 && !opt->two_phase)
> {
> pg_log_warning("two_phase option will not be enabled for
> replication slots");
> pg_log_warning_detail("Subscriptions will be created with the
> two_phase option disabled. "
> "Prepared transactions will be
> replicated at COMMIT PREPARED.");
> pg_log_warning_hint("You can use '--enable-two_phase' switch
> to enable two_phase.");
> }
>
> ~~~
>
> But, check what other people think just in case I am in the minority
> by thinking these warnings in docs/code are still potentially useful.
>

I agree with your suggestions. I have used your suggestions and added
them to the latest patch.
The v11 version patch attached at [1] has the changes for the same.

[1] - https://www.postgresql.org/message-id/CAHv8Rj%2BZqc9qVEaJDW03Cux_S_CMHWNM056qqJi5%2Bz9vSYgtew%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.