On Mon, Oct 23, 2023 at 5:52 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On 10/20/23 5:27 AM, shveta malik wrote:
> > On Wed, Oct 18, 2023 at 4:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > PFA v25 patch set. The changes are:
> >
> > 1) 'enable_failover' is changed to 'failover'
> > 2) Alter subscription changes to support 'failover'
> > 3) Fixes a bug in patch001 wherein any change in standby_slot_names
> > was not considered in the flow where logical walsenders wait for
> > standby's confirmation. Now during the wait, if standby_slot_names is
> > changed, wait is restarted using new standby_slot_names.
> > 4) Addresses comments by Bertrand and Amit in [1],[2],[3]
> >
> > The changes are mostly in patch001 and a very few in patch002.
> >
> > Thank You Ajin for working on alter-subscription changes and adding
> > more TAP-tests for 'failover'
> >
>
> Thanks for updating the patch!
>
> Looking at 0001 and doing some experiment:
>
> Creating a logical slot with failover = true and then launching
> pg_logical_slot_get_changes() or pg_recvlogical() on it results
> to setting failover back to false.
>
> It occurs while creating the decoding context here:
>
> @@ -602,6 +602,9 @@ CreateDecodingContext(XLogRecPtr start_lsn,
> SnapBuildSetTwoPhaseAt(ctx->snapshot_builder, start_lsn);
> }
>
> + /* set failover in the slot, as requested */
> + slot->data.failover = ctx->failover;
> +
>
> I think we can get rid of this change in CreateDecodingContext().
>
Thanks for pointing it out. I will correct it in the next patch.
> Looking at 0002:
>
> /* Enter main loop */
> for (;;)
> {
> int rc;
> long wait_time = DEFAULT_NAPTIME_PER_CYCLE;
>
> CHECK_FOR_INTERRUPTS();
>
> /*
> * If it is Hot standby, then try to launch slot-sync workers else
> * launch apply workers.
> */
> if (RecoveryInProgress())
> {
> /* Launch only if we have succesfully made the connection */
> if (wrconn)
> LaunchSlotSyncWorkers(&wait_time, wrconn);
> }
>
> We are waiting for DEFAULT_NAPTIME_PER_CYCLE (3 minutes) before checking if there
> is new synced slot(s) to be created on the standby. Do we want to keep this behavior
> for V1?
>
I think for the slotsync workers case, we should reduce the naptime in
the launcher to say 30sec and retain the default one of 3mins for
subscription apply workers. Thoughts?
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com