On Thu, 20 Apr 2023 at 11:01, Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thank you for reviewing! PSA new patchset.
>
> > > Additionally, I added a checking functions in 0003
> > > According to pg_resetwal and other functions, the length of
> > CHECKPOINT_SHUTDOWN
> > > record seems (SizeOfXLogRecord + SizeOfXLogRecordDataHeaderShort +
> > sizeof(CheckPoint)).
> > > Therefore, the function ensures that the difference between current insert
> > position
> > > and confirmed_flush_lsn is less than (above + page header).
> >
> > Logical replication slots can be created only if wal_level >= logical,
> > currently we do not have any check to see if wal_level >= logical if
> > "--include-logical-replication-slots" option is specified. If
> > include-logical-replication-slots is specified with pg_upgrade, we
> > will be creating replication slots after a lot of steps like
> > performing prechecks, analyzing, freezing, deleting, restoring,
> > copying, setting related objects and then create replication slot,
> > where we will be erroring out after a lot of time(Many cases
> > pg_upgrade takes a lot of hours to perform these operations). I feel
> > it would be better to add a check in the beginning itself somewhere in
> > check_new_cluster to see if wal_level is set appropriately in case of
> > include-logical-replication-slot option to detect and throw an error
> > early itself.
>
> I see your point. Moreover, I think max_replication_slots != 0 must be also
> checked.
> I added a checking function and related test in 0001.
Thanks for the updated patch.
A Few comments:
1) if the verbose option is enabled, we should print the new slot
information, we could add a function print_slot_infos similar to
print_rel_infos which could print slot name and two_phase is enabled
or not.
+ end_progress_output();
+ check_ok();
+
+ /* update new_cluster info now that we have objects in the databases */
+ get_db_and_rel_infos(&new_cluster);
+}
2) Since we will be using this option with pg_upgrade, should we use
this along with the --binary-upgrade option only?
+ if (dopt.logical_slots_only && dopt.dataOnly)
+ pg_fatal("options --logical-replication-slots-only and
-a/--data-only cannot be used together");
+ if (dopt.logical_slots_only && dopt.schemaOnly)
+ pg_fatal("options --logical-replication-slots-only and
-s/--schema-only cannot be used together");
3) Since it two_phase is boolean, can we use bool data type instead of string:
+ slotinfo[i].dobj.objType = DO_LOGICAL_REPLICATION_SLOT;
+
+ slotinfo[i].dobj.catId.tableoid = InvalidOid;
+ slotinfo[i].dobj.catId.oid = InvalidOid;
+ AssignDumpId(&slotinfo[i].dobj);
+
+ slotinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i,
i_slotname));
+
+ slotinfo[i].plugin = pg_strdup(PQgetvalue(res, i, i_plugin));
+ slotinfo[i].twophase = pg_strdup(PQgetvalue(res, i,
i_twophase));
We can change it to something like:
if (strcmp(PQgetvalue(res, i, i_twophase), "t") == 0)
slotinfo[i].twophase = true;
else
slotinfo[i].twophase = false;
4) The comments are inconsistent, some have termination characters and
some don't. We can keep it consistent:
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
+
+# Initialize old publisher node
+my $old_publisher = PostgreSQL::Test::Cluster->new('old_publisher');
+$old_publisher->init(allows_streaming => 'logical');
+$old_publisher->start;
+
+# Initialize subscriber node
+my $subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$subscriber->init(allows_streaming => 'logical');
+$subscriber->start;
+
+# Schema setup
+$old_publisher->safe_psql('postgres',
+ "CREATE TABLE tbl AS SELECT generate_series(1,10) AS a");
+$subscriber->safe_psql('postgres', "CREATE TABLE tbl (a int)");
+
+# Initialize new publisher node
5) should we use free instead of pfree as used in other function like
dumpForeignServer:
+ appendPQExpBuffer(query, ");");
+
+ ArchiveEntry(fout, slotinfo->dobj.catId, slotinfo->dobj.dumpId,
+ ARCHIVE_OPTS(.tag = slotname,
+
.description = "REPLICATION SLOT",
+
.section = SECTION_POST_DATA,
+
.createStmt = query->data));
+
+ pfree(slotname);
+ destroyPQExpBuffer(query);
+ }
+}
Regards,
Vignesh
