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

On Mon, 22 May 2023 at 15:50, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.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