On Sat, Nov 11, 2023 at 5:46 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Nov 10, 2023 at 03:27:25PM +0530, Amit Kapila wrote:
> > I don't think this comment is correct because there won't be any apply
> > activity on the new cluster as after restoration subscriptions should
> > be disabled. On the old cluster, I think one problem is that the
> > origins may move forward after we copy them which can cause data
> > inconsistency issues. The other is that we may not prefer to generate
> > additional data and WAL during the upgrade. Also, I am not completely
> > sure about using the word 'corruption' in this context.
>
> What is your suggestion here? Would it be better to just aim for
> simplicity and just say that we don't want it to run because "it can
> lead to some inconsistent behaviors"?
>
I think we can be specific about logical replication stuff. I have not
done any study on autovacuum behavior related to this, so we can
update about it separately if required. I could think of something
like the following:
- /* Use -b to disable autovacuum. */
+ /*
+ * Use -b to disable autovacuum and logical replication launcher
+ * (effective in PG17 or later for the latter).
+ *
+ * Logical replication workers can stream data during the
upgrade which can
+ * cause replication origins to move forward after we have copied them.
+ * It can cause the system to request the data which is already present
+ * in the new cluster.
+ */
Now, ideally, such a comment change makes more sense along with the
main patch, so either we can go without this comment change or
probably wait till the main patch is ready and merge just before it or
along with it. I am fine either way.
BTW, it is not clear to me another part of the comment "... for the
latter" in the proposed wording. Is there any typo there or am I
missing something?
--
With Regards,
Amit Kapila.