RE: Synchronizing slots from primary to standby

On Thursday, November 23, 2023 6:06 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Tue, Nov 21, 2023 at 8:32 PM shveta malik <shveta(dot)malik(at)gmail(dot)com>
> wrote:
> >
> > v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd,
> > rebased the patches. PFA v37_2 patches.
>
> Thanks for the patch. Some comments:

Thanks for the comments.

> subscriptioncmds.c:
> CreateSubscription()
> and tablesync.c:
> process_syncing_tables_for_apply()
> walrcv_create_slot(wrconn, opts.slot_name, false,
> twophase_enabled,
> - CRS_NOEXPORT_SNAPSHOT, NULL);
> -
> - if (twophase_enabled)
> - UpdateTwoPhaseState(subid,
> LOGICALREP_TWOPHASE_STATE_ENABLED);
> -
> + failover_enabled,
> CRS_NOEXPORT_SNAPSHOT, NULL);
>
> either here or in libpqrcv_create_slot(), shouldn't you check the remote server
> version if it supports the failover flag?

I think we expect the create slot to fail if the server doesn't support failover.
The same is true for two_phase option.

>
>
> +
> + /*
> + * If only the slot_name is specified, it is possible
> that the user intends to
> + * use an existing slot on the publisher, so here we
> enable failover for the
> + * slot if requested.
> + */
> + else if (opts.slot_name && failover_enabled)
> + {
> + walrcv_alter_slot(wrconn, opts.slot_name, opts.failover);
> + ereport(NOTICE,
> + (errmsg("enabled failover for replication
> slot \"%s\" on publisher",
> + opts.slot_name)));
> + }
>
> Here, the code only alters the slot if failover = true. You could use "else if
> (opts.slot_name && IsSet(opts.specified_opts, SUBOPT_FAILOVER)" to check if
> the failover flag is specified and alter for failover=false as well.

Adjusted.

> Also, shouldn't
> you check for the server version if the command ALTER_REPLICATION_SLOT is
> supported?

Similar to create_slot, we expect the command fail if the target server doesn't
support failover the user specified failover = true.

>
> slot.c:
> ReplicationSlotAlter()
>
> +void
> +ReplicationSlotAlter(const char *name, bool failover) {
> + Assert(MyReplicationSlot == NULL);
> +
> + ReplicationSlotAcquire(name, true);
> +
> + if (SlotIsPhysical(MyReplicationSlot))
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot use %s with a physical replication slot",
> + "ALTER_REPLICATION_SLOT"));
>
> shouldn't you release the slot by calling ReplicationSlotRelease before
> erroring out?

No, I think the release of the replication slot will be handled by either
WalSndErrorCleanup, ReplicationSlotShmemExit, or the ReplicationSlotRelease
call in PostgresMain().

>
> slot.c:
> +/*
> + * A helper function to validate slots specified in standby_slot_names GUCs.
> + */
> +static bool
> +validate_standby_slots(char **newval)
> +{
> + char *rawname;
> + List *elemlist;
> + ListCell *lc;
> +
> + /* Need a modifiable copy of string */
> + rawname = pstrdup(*newval);
>
> rawname is not always freed.

The code has been changed due to other comments.

>
> launcher.c:
>
> + SlotSyncWorker->hdr.proc = MyProc;
> +
> + before_shmem_exit(slotsync_worker_detach, (Datum) 0);
> +
> + LWLockRelease(SlotSyncWorkerLock);
> +}
>
> before_shmem_exit() can error out leaving the lock acquired. Maybe you
> should release the lock prior to calling before_shmem_exit() because you don't
> need the lock there.

This has been fixed.

Best Regards,
Hou zj