Re: [PoC] pg_upgrade: allow to upgrade publisher node

On Mon, 22 May 2023 at 15:50, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Wang,
>
> Thank you for reviewing! PSA new version.
>
> > For patches 0001
> >
> > 1. The latest patch set fails to apply because the new commit (0245f8d) in HEAD.
>
> I didn't notice that. Thanks, fixed.
>
> > 2. In file pg_dump.h.
> > ```
> > +/*
> > + * The LogicalReplicationSlotInfo struct is used to represent replication
> > + * slots.
> > + *
> > + * XXX: add more attributes if needed
> > + */
> > +typedef struct _LogicalReplicationSlotInfo
> > +{
> > + DumpableObject dobj;
> > + char *plugin;
> > + char *slottype;
> > + bool twophase;
> > +} LogicalReplicationSlotInfo;
> > ```
> >
> > Do we need the structure member "slottype"? It seems we do not use "slottype"
> > because we only dump logical replication slot.
>
> As you said, this attribute is not needed. This is a garbage of previous efforts.
> Removed.
>
> > For patch 0002
> >
> > 3. In the function SaveSlotToPath
> > ```
> > - /* and don't do anything if there's nothing to write */
> > - if (!was_dirty)
> > + /*
> > + * and don't do anything if there's nothing to write, unless it's this is
> > + * called for a logical slot during a shutdown checkpoint, as we want to
> > + * persist the confirmed_flush_lsn in that case, even if that's the only
> > + * modification.
> > + */
> > + if (!was_dirty && !is_shutdown && !SlotIsLogical(slot))
> > ```
> > It seems that the code isn't consistent with our expectation.
> > If this is called for a physical slot during a shutdown checkpoint and there's
> > nothing to write, I think it will also persist physical slots to disk.
>
> You meant to say that we should not change handlings for physical case, right?
>
> > For patch 0003
> >
> > 4. In the function check_for_parameter_settings
> > ```
> > + /* --include-logical-replication-slots can be used since PG 16. */
> > + if (GET_MAJOR_VERSION(new_cluster->major_version < 1600))
> > + return;
> > ```
> > It seems that there is a slight mistake (the input of GET_MAJOR_VERSION) in the
> > if-condition:
> > GET_MAJOR_VERSION(new_cluster->major_version < 1600)
> > ->
> > GET_MAJOR_VERSION(new_cluster->major_version) <= 1500
> >
> > Please also check the similar if-conditions in the below two functions
> > check_for_confirmed_flush_lsn (in 0003 patch)
> > check_are_logical_slots_active (in 0004 patch)
>
> Done. I grepped with GET_MAJOR_VERSION, and confirmed they were fixed.

Few minor comments:
1) we could remove the variable slotname from the below code by using
PQgetvalue directly in pg_log:
+ for (i = 0; i < ntups; i++)
+ {
+ char *slotname;
+
+ is_error = true;
+
+ slotname = PQgetvalue(res, i, i_slotname);
+
+ pg_log(PG_WARNING,
+ "\nWARNING: logical replication slot \"%s\"
is not active",
+ slotname);
+ }

2) This include "catalog/pg_control.h" should be after inclusion pg_collation.h
#include "catalog/pg_authid_d.h"
+#include "catalog/pg_control.h"
#include "catalog/pg_collation.h"

3) This spurious addition line change might not be required in this patch:
--- a/src/bin/pg_upgrade/t/003_logical_replication_slots.pl
+++ b/src/bin/pg_upgrade/t/003_logical_replication_slots.pl
@@ -85,11 +85,39 @@ $old_node->safe_psql(
]);

my $result = $old_node->safe_psql('postgres',
- "SELECT count (*) FROM
pg_logical_slot_get_changes('test_slot', NULL, NULL)"
+ "SELECT count(*) FROM
pg_logical_slot_peek_changes('test_slot', NULL, NULL)"
);
+
is($result, qq(12), 'ensure WALs are not consumed yet');
$old_node->stop;

4) This inclusion "#include "access/xlogrecord.h" is not required:
#include "postgres_fe.h"

+#include "access/xlogrecord.h"
+#include "access/xlog_internal.h"
#include "catalog/pg_authid_d.h"

5)"thepublisher's" should be "the publisher's"
When a live check is requested, there is a possibility of additional changes
occurring, which may cause the current WAL position to exceed the
confirmed_flush_lsn
of the slot. As a result, we check the confirmed_flush_lsn of each logical slot
instead. This is sufficient as all the WAL records will be sent during
thepublisher's
shutdown.

Regards,
Vignesh