On 8/1/23 5:39 AM, Amit Kapila wrote:
On Fri, Jul 28, 2023 at 5:48 PM vignesh C <vignes...@gmail.com> wrote:

Here is a patch which checks that there are no WAL records other than
CHECKPOINT_SHUTDOWN WAL record to be consumed based on the discussion
from [1].


Few comments:
=============

2.
+ if (dopt.logical_slots_only)
+ {
+ if (!dopt.binary_upgrade)
+ pg_fatal("options --logical-replication-slots-only requires option
--binary-upgrade");
+
+ if (dopt.dataOnly)
+ pg_fatal("options --logical-replication-slots-only and
-a/--data-only cannot be used together");
+
+ if (dopt.schemaOnly)
+ pg_fatal("options --logical-replication-slots-only and
-s/--schema-only cannot be used together");

Can you please explain why the patch imposes these restrictions? I
guess the binary_upgrade is because you want this option to be used
for the upgrade. Do we want to avoid giving any other option with
logical_slots, if so, are the above checks sufficient and why?

Can I take this a step further on the user interface and ask why the flag would be "--include-logical-replication-slots" vs. being enabled by default?

Are there reasons why we wouldn't enable this feature by default on pg_upgrade, and instead (if need be) have a flag that would be "--exclude-logical-replication-slots"? Right now, not having the ability to run pg_upgrade with logical replication slots enabled on the publisher is a a very big pain point for users, so I would strongly recommend against adding friction unless there is a very large challenge with such an implementation.

Thanks,

Jonathan

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

Reply via email to