On Mon, Nov 13, 2023 at 08:45:12AM +0530, Amit Kapila wrote:
> 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.
Autovacuum, as far as I recall, could decide to do some work before
files are physically copied from the old to the new cluster,
corrupting the new cluster. Per 76dd09bbec89:
+ * If we have lost the autovacuum launcher, try to start a new one.
+ * We don't want autovacuum to run in binary upgrade mode because
+ * autovacuum might update relfrozenxid for empty tables before
+ * the physical files are put in place.
> - /* 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.
Another location would be to document that stuff directly in
launcher.c where the check for IsBinaryUpgrade would be added. You
are right that it makes little sense to document that now, so how
about:
1) keeping pg_upgrade.c minimal, say:
- /* Use -b to disable autovacuum. */
+ /*
+ * Use -b to disable autovacuum and logical replication
+ * launcher (in 17~).
+ */
With a removal of the comment block related to
max_logical_replication_workers=0?
2) Document that in ApplyLauncherRegister() as part of the main patch
for the subscribers?
> 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?
The "latter" refers to the logirep launcher here, as -b would affect
it only in 17~ with the patch I sent previously.
--
Michael