On Fri, Sep 15, 2023 at 6:32 PM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
>
> > Thank you for reviewing! PSA new version patch set.
>
> Sorry, wrong patch attached. PSA the correct ones.
> There is a possibility that XLOG_PARAMETER_CHANGE may be generated, when GUC
> parameters are changed just before doing the upgrade. Added to list.
>
You forgot to update 0002 patch for XLOG_PARAMETER_CHANGE. I think it
is okay to move walinspect's functionality into common place so that
it can be used by this patch as suggested by Hou-San. The only reason
it is okay to keep it specific to walinspect is if we want to enhance
that functions for walinspect but I think if that happens then we can
evaluate whether to enhance it by having additional parameters or
creating something specific for walinspect.
* +Datum
+binary_upgrade_validate_wal_record_types_after_lsn(PG_FUNCTION_ARGS)
How about naming it as binary_upgrade_validate_wal_records()? I don't
see it is helpful to make it too long.
Apart from this, I have made minor cosmetic changes in the attached.
If these looks okay to you then you can include them in next version.
--
With Regards,
Amit Kapila.
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index 2588d6d7b8..1a17572d14 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -418,7 +418,7 @@ make prefix=/usr/local/pgsql.new install
</listitem>
<listitem>
<para>
- Old cluster has replicated all the changes replicated to subscribers.
+ The old cluster has replicated all the changes to subscribers.
</para>
</listitem>
<listitem>
diff --git a/src/backend/utils/adt/pg_upgrade_support.c
b/src/backend/utils/adt/pg_upgrade_support.c
index c4b2980a8a..2914b2833e 100644
--- a/src/backend/utils/adt/pg_upgrade_support.c
+++ b/src/backend/utils/adt/pg_upgrade_support.c
@@ -12,9 +12,7 @@
#include "postgres.h"
#include "access/heapam_xlog.h"
-#include "access/rmgr.h"
#include "access/xlog.h"
-#include "access/xlog_internal.h"
#include "access/xlogutils.h"
#include "catalog/binary_upgrade.h"
#include "catalog/heap.h"
@@ -279,17 +277,13 @@ binary_upgrade_set_missing_value(PG_FUNCTION_ARGS)
* This function is used to verify that there are no WAL records (except some
* types) after confirmed_flush_lsn of logical slots, which means all the
* changes were replicated to the subscriber. There is a possibility that some
- * WALs are inserted after logical waslenders exit, so such types would be
- * ignored.
+ * WALs are inserted during upgrade, so such types would be ignored.
*
* XLOG_CHECKPOINT_SHUTDOWN is ignored because it would be inserted after the
- * waslender exits. Moreover, the following types of records would be during
- * the pg_upgrade --check, so they are ignored too.
- *
- * - XLOG_CHECKPOINT_ONLINE
- * - XLOG_RUNNING_XACTS
- * - XLOG_FPI_FOR_HINT
- * - XLOG_HEAP2_PRUNE
+ * waslender exits. Moreover, the following types of records could be generated
+ * during the pg_upgrade --check, so they are ignored too:
+ * XLOG_CHECKPOINT_ONLINE, XLOG_RUNNING_XACTS, XLOG_FPI_FOR_HINT,
+ * XLOG_HEAP2_PRUNE, XLOG_PARAMETER_CHANGE.
*/
Datum
binary_upgrade_validate_wal_record_types_after_lsn(PG_FUNCTION_ARGS)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index df1ce67fc0..37c75bd024 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -1399,8 +1399,8 @@ check_for_user_defined_encoding_conversions(ClusterInfo
*cluster)
/*
* check_new_cluster_logical_replication_slots()
*
- * Make sure there are no logical replication slots on the new cluster and that
- * the parameter settings necessary for creating slots are sufficient.
+ * Verify that there are no logical replication slots on the new cluster and
+ * that the parameter settings necessary for creating slots are sufficient.
*/
static void
check_new_cluster_logical_replication_slots(void)
@@ -1476,12 +1476,8 @@ check_new_cluster_logical_replication_slots(void)
/*
* check_old_cluster_for_valid_slots()
*
- * Make sure logical replication slots can be migrated to new cluster.
- * Following points are checked:
- *
- * - All logical replication slots are usable.
- * - All logical replication slots consumed all WALs, except some
acceptable
- * types.
+ * Verify that all the logical slots are usable and consumed all the WAL
+ * before shutdown.
*/
static void
check_old_cluster_for_valid_slots(bool live_check)
@@ -1521,8 +1517,8 @@ check_old_cluster_for_valid_slots(bool live_check)
}
/*
- * Do additional checks to ensure that all logical
replication
- * slots have reached the current WAL position.
+ * Do additional check to ensure that all logical
replication slots
+ * have consumed all the WAL before shutdown.
*
* Note: This can be satisfied only when the old
cluster has been
* shut down, so we skip this for live checks.
