On Thu, Sep 21, 2023 at 6:54 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > 6.
> > + if (PQntuples(res) != 1)
> > + pg_fatal("could not count the number of logical replication slots");
> > +
> >
> > Not existing a single logical replication slot an error? I think it
> > must be if (PQntuples(res) == 0) return;?
> >
>
> The query executes "SELECT count(*)...", IIUC it exactly returns 1 row.
Ah, got it.
> > 7. A nit:
> > + nslots_on_new = atoi(PQgetvalue(res, 0, 0));
> > +
> > + if (nslots_on_new)
> >
> > Just do if(atoi(PQgetvalue(res, 0, 0)) > 0) and get rid of nslots_on_new?
>
> Note that the vaule would be used for upcoming pg_fatal. I prefer current style
> because multiple atoi(PQgetvalue(res, 0, 0)) was not so beautiful.
+1.
> You mentioned at line 118, but at that time logical replication system is not created.
> The subscriber is created at line 163.
> Therefore WALs would not be consumed automatically.
So, not calling pg_logical_slot_get_changes() on test_slot1 won't
consume the WAL?
A few more comments:
1.
+ /*
+ * Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the
+ * checkpointer process. If WALs required by logical replication slots
+ * are removed, the slots are unusable. This setting prevents the
+ * invalidation of slots during the upgrade. We set this option when
IIUC, during upgrade we don't want the checkpointer to remove WAL that
may be needed by logical slots, for that the patch overrides the user
set value for max_slot_wal_keep_size. What if the WAL is removed
because of the wal_keep_size setting?
2.
+++ b/src/bin/pg_upgrade/t/003_logical_replication_slots.pl
How about a more descriptive and pointed name for the TAP test file,
something like 003_upgrade_logical_replication_slots.pl?
3. Does this patch support upgrading of logical replication slots on a
streaming standby? If yes, isn't it a good idea to add one test for
upgrading standby with logical replication slots?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
