Thanks Shveta for all your comments. I'll prepare a version 2 of the patch
including your proposals.
Regards
Fabrice
On Thu, 11 Sep 2025, 07:42 shveta malik <shveta.ma...@gmail.com> wrote:
> On Sun, Sep 7, 2025 at 1:32 PM Fabrice Chapuis <fabrice636...@gmail.com>
> wrote:
> >
> > Hi,
> > Here the version 1 of the patch with the modifications.
> > I do a git diff --check until there are no more errors.
> > In a next version of the patch, I think I have make change to call
> ReplicationSlotCreate() function with the value of the flag
> allow_overwrite be consistent
>
> In which flow? I see all ReplicationSlotCreate() references already
> accepting the value.
>
> > and modify the interface ReplicationSlotCreate to display the attribute
> allow_overwrite.
> >
>
> Do you mean to modify pg_replication_slots?
>
> Thank You for the patch. Please find a few comments:
>
> 1)
> In both create_physical_replication_slot() and
> create_physical_replication_slot():
> + false, false,false);
>
> ,false --> , false (space after comma is recommended)
>
> 2)
> + elog(DEBUG1, "logical replication slot %s created with option
> allow_overwrite to %s",
> + NameStr(slot->data.name), slot->data.allow_overwrite ? "true" :
> "false");
>
> IMO, we don't need this as we do not log other properties of the slot as
> well.
>
> 3)
> We can have pg_replication_slots (pg_get_replication_slots) modified
> to display the new property. Also we can modify docs to have the new
> property defined.
>
> 4)
> + {
> + /* Check if we need to overwrite an existing logical slot */
> + if (allow_overwrite)
> + {
> + /* Get rid of a replication slot that is no longer wanted */
> + ReplicationSlotDrop(remote_slot->name,true);
> +
> + /* Get rid of a replication slot that is no longer wanted */
> + ereport(WARNING,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("slot \"%s\" already exists"
> + " on the standby but it will be dropped because "
> + "flag allow_overwrite is set to true",
> + remote_slot->name));
> +
> + /* Going back to the main loop after droping the failover slot */
> + return false;
>
> Currently we are returning after dropping the slot. But I think we
> shall drop the slot and proceed with new slot creation of the same
> name otherwise we may be left with old slot dropped and no new slot
> created specially when API is run.
>
> Scenario:
> --On primary, create 2 slots: slot1 and slot2.
> --On standby, create one slot slot1 with allow_overwrite=true.
> --Run 'SELECT pg_sync_replication_slots();' on standby.
>
> At the end of API, expectation is that both slots will be present with
> 'synced'=true, but only slot2 is present. if we run this API next
> time, slot1 is created. It should have been dropped and recreated (or
> say overwritten) in the first run itself.
>
> 5)
> IMO, LOG is sufficient here, as the action aligns with the
> user-provided 'allow_overwrite' setting.
>
> thanks
> Shveta
>
