On Fri, Jun 26, 2026 at 10:49 AM vignesh C <[email protected]> wrote: > > On Thu, 25 Jun 2026 at 19:15, Dilip Kumar <[email protected]> wrote: > > > > On Thu, Jun 25, 2026 at 6:37 PM Amit Kapila <[email protected]> wrote: > > > > > > On Thu, Jun 25, 2026 at 6:30 PM Dilip Kumar <[email protected]> wrote: > > > > > > > > On Thu, Jun 25, 2026 at 4:57 PM Amit Kapila <[email protected]> > > > > wrote: > > > > > > > > > > > > > > 2. > > > > > @@ -2482,6 +2681,8 @@ DropSubscription(DropSubscriptionStmt *stmt, > > > > > bool isTopLevel) > > > > > deleteDependencyRecordsFor(SubscriptionRelationId, subid, false); > > > > > deleteSharedDependencyRecordsFor(SubscriptionRelationId, subid, 0); > > > > > > > > > > + drop_sub_conflict_log_table(subid, subname, subconflictlogrelid); > > > > > > > > > > It would be better to drop the table before cleaning up the dependency > > > > > record. Right now, it is okay even in current order because dependency > > > > > removal is trying to remove where subid is depender. > > > > > > > > So whats your suggestion change it now or not? I feel either way is > > > > fine. > > > > > > > > > > Let's change now itself. > > > > PFA, updated version of the patch, fixes all the comments of V57 > > Thanks for the updated patches. > Currently, we allow an empty string for conflict_log_destination: > postgres=# CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres > host=localhost port=5432' PUBLICATION pub1 WITH > (conflict_log_destination = ''); > NOTICE: created replication slot "sub1" on publisher > CREATE SUBSCRIPTION > > postgres=# SELECT subconflictlogdest FROM pg_subscription WHERE > subname = 'sub1'; > subconflictlogdest > -------------------- > log > (1 row) > > However, this is inconsistent with the behavior of the other > subscription options, which all reject an empty string. For example: > postgres=# CREATE SUBSCRIPTION sub2 ... WITH (origin = ''); > ERROR: unrecognized origin value: "" > > postgres=# CREATE SUBSCRIPTION sub2 ... WITH (streaming = ''); > ERROR: streaming requires a Boolean value or "parallel" > > postgres=# CREATE SUBSCRIPTION sub2 ... WITH (copy_data = ''); > ERROR: copy_data requires a Boolean value > > postgres=# CREATE SUBSCRIPTION sub2 ... WITH (wal_receiver_timeout = ''); > ERROR: invalid value for parameter "wal_receiver_timeout": "" > > postgres=# CREATE SUBSCRIPTION sub2 ... WITH (max_retention_duration = ''); > ERROR: max_retention_duration requires an integer value > > postgres=# CREATE SUBSCRIPTION sub2 ... WITH (slot_name = ''); > ERROR: replication slot name "" is too short > > I think conflict_log_destination should behave consistently with the > other subscription options and reject an empty string rather than > silently treating it as log. If there is a specific reason for > accepting '', it would be good to document that behavior. >
I changed this in the latest version of the patch. -- Regards, Dilip Kumar Google
