On Fri, Sep 26, 2025 at 9:52 PM Fabrice Chapuis <fabrice636861@gmail.com> wrote: > > Hi Shveta, > > Here is the version 1 of the patch with corrections >
Thanks. We can bump the version to v2 now — that is, each time we post a new version, we can increment the version number.
The patch fails to apply to the latest HEAD. Also it had some 'trailing whitespace' issues. Can you please rebase the patch and post it again?
> 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. > Point 1-2 done > 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. > > Point 3 Not yet implemented > > 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. > > Point 4: > I put the creation slot under the allow_overwrite condition. > After testing with syn_standby_slots disable on the standby, it works. I think the code for the synchronisation of the new slot could be factorised. > > 5) > IMO, LOG is sufficient here, as the action aligns with the > user-provided 'allow_overwrite' setting. > > Point 5 > Where is it in the code? >
This one:
+ ereport(WARNING, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("slot \"%s\" already exists" + " on the standby but try to recreate it because " + "flag allow_overwrite is set to true", + remote_slot->name));
