Re: [PoC] pg_upgrade: allow to upgrade publisher node - Mailing list pgsql-hackers

Hi Kuroda-san. Here are some review comments for the v10-0001 patch.

======

General.

1. pg_dump option is documented to the user.

I'm not sure about exposing the new pg_dump
--logical-replication-slots-only option to the user.

I thought this pg_dump option was intended only to be called
*internally* by the pg_upgrade.
But, this patch is also documenting the new option for the user (in
case they want to call it independently?)

Maybe exposing it  is OK, but if you do that then I thought perhaps
there should also be some additional pg_dump tests just for this
option (i.e. tested independently of the pg_upgrade)

======
Commit message

2.
For pg_upgrade, when '--include-logical-replication-slots' is
specified, it executes
pg_dump with the new "--logical-replication-slots-only" option and
restores from the
dump. Apart from restoring schema, pg_resetwal must not be called
after restoring
replication slots. This is because the command discards WAL files and
starts from a
new segment, even if they are required by replication slots. This
leads to an ERROR:
"requested WAL segment XXX has already been removed". To avoid this,
replication slots
are restored at a different time than other objects, after running pg_resetwal.

~~

The "Apart from" sentence maybe could do with some rewording. I
noticed there is a code comment (below fragment) that says the same as
this, but more clearly. Maybe it is better to use that code-comment
wording in the comment message.

+ * XXX We cannot dump replication slots at the same time as the schema
+ * dump because we need to separate the timing of restoring
+ * replication slots and other objects. Replication slots, in
+ * particular, should not be restored before executing the pg_resetwal
+ * command because it will remove WALs that are required by the slots.

======
src/bin/pg_dump/pg_dump.c

3. main

+ if (dopt.logical_slots_only && !dopt.binary_upgrade)
+ pg_fatal("options --logical-replication-slots-only requires option
--binary-upgrade");
+
+ 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");
+

Consider if it might be simpler to combine together all those
dopt.logical_slots_only checks.

SUGGESTION

if (dopt.logical_slots_only)
{
    if (!dopt.binary_upgrade)
        pg_fatal("options --logical-replication-slots-only requires
option --binary-upgrade");

    if (dopt.dataOnly)
        pg_fatal("options --logical-replication-slots-only and
-a/--data-only cannot be used together");
    if (dopt.schemaOnly)
        pg_fatal("options --logical-replication-slots-only and
-s/--schema-only cannot be used together");
}

~~~

4. getLogicalReplicationSlots

+ /* Check whether we should dump or not */
+ if (fout->remoteVersion < 160000 || !dopt->logical_slots_only)
+ return;

I'm not sure if this check is necessary. Given the way this function
is called, is it possible for this check to fail? Maybe that quick
exit would be better code as an Assert?

~~~

5. dumpLogicalReplicationSlot

+dumpLogicalReplicationSlot(Archive *fout,
+    const LogicalReplicationSlotInfo *slotinfo)
+{
+ DumpOptions *dopt = fout->dopt;
+
+ if (!dopt->logical_slots_only)
+ return;

(Similar to the previous comment). Is it even possible to arrive here
when dopt->logical_slots_only is false. Maybe that quick exit would be
better coded as an Assert?

~

6.
+ PQExpBuffer query = createPQExpBuffer();
+ char    *slotname = pg_strdup(slotinfo->dobj.name);

I wondered if it was really necessary to strdup/free this slotname.
e.g. And if it is, then why don't you do this for the slotinfo->plugin
field?

======
src/bin/pg_upgrade/check.c

7. check_and_dump_old_cluster

  /* Extract a list of databases and tables from the old cluster */
  get_db_and_rel_infos(&old_cluster);
+ get_logical_slot_infos(&old_cluster);

Is it correct to associate this new call with that existing comment
about "databases and tables"?

~~~

8. check_new_cluster

@@ -188,6 +190,7 @@ void
 check_new_cluster(void)
 {
  get_db_and_rel_infos(&new_cluster);
+ get_logical_slot_infos(&new_cluster);

  check_new_cluster_is_empty();

@@ -210,6 +213,9 @@ check_new_cluster(void)
  check_for_prepared_transactions(&new_cluster);

  check_for_new_tablespace_dir(&new_cluster);
+
+ if (user_opts.include_logical_slots)
+ check_for_parameter_settings(&new_cluster);

Can the get_logical_slot_infos() be done later, guarded by that the
same condition if (user_opts.include_logical_slots)?

~~~

9. check_new_cluster_is_empty

+ * If --include-logical-replication-slots is required, check the
+ * existing of slots
+ */

Did you mean to say "check the existence of slots"?

~~~

10. check_for_parameter_settings

+ if (strcmp(wal_level, "logical") != 0)
+ pg_fatal("wal_level must be \"logical\", but set to \"%s\"",
+ wal_level);

/but set to/but is set to/


======
src/bin/pg_upgrade/info.c

11. get_db_and_rel_infos

+ {
  get_rel_infos(cluster, &cluster->dbarr.dbs[dbnum]);

+ /*
+ * Additionally, slot_arr must be initialized because they will be
+ * checked later.
+ */
+ cluster->dbarr.dbs[dbnum].slot_arr.nslots = 0;
+ cluster->dbarr.dbs[dbnum].slot_arr.slots = NULL;
+ }

11a.
I think probably it would have been easier to just use 'pg_malloc0'
instead of 'pg_malloc' in the get_db_infos, then this code would not
be necessary.

~

11b.
BTW, shouldn't this function also be calling free_logical_slot_infos()
too? That will also have the same effect (initializing the slot_arr)
but without having to change anything else.

~~~

12. get_logical_slot_infos
+/*
+ * Higher level routine to generate LogicalSlotInfoArr for all databases.
+ */
+void
+get_logical_slot_infos(ClusterInfo *cluster)

To be consistent with the other nearby function headers it should have
another line saying just get_logical_slot_infos().

~~~

13. get_logical_slot_infos

+void
+get_logical_slot_infos(ClusterInfo *cluster)
+{
+ int dbnum;
+
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ if (cluster->dbarr.dbs[dbnum].slot_arr.slots)
+ free_logical_slot_infos(&cluster->dbarr.dbs[dbnum].slot_arr);
+
+ get_logical_slot_infos_per_db(cluster, &cluster->dbarr.dbs[dbnum]);
+ }
+
+ if (cluster == &old_cluster)
+ pg_log(PG_VERBOSE, "\nsource databases:");
+ else
+ pg_log(PG_VERBOSE, "\ntarget databases:");
+
+ if (log_opts.verbose)
+ {
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ pg_log(PG_VERBOSE, "Database: %s", cluster->dbarr.dbs[dbnum].db_name);
+ print_slot_infos(&cluster->dbarr.dbs[dbnum].slot_arr);
+ }
+ }
+}

I didn't see why there are 2 loops exactly the same. I think with some
minor refactoring these can both be done in the same loop can't they?

SUGGESTION 1:

if (cluster == &old_cluster)
    pg_log(PG_VERBOSE, "\nsource databases:");
else
    pg_log(PG_VERBOSE, "\ntarget databases:");

for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
{
    if (cluster->dbarr.dbs[dbnum].slot_arr.slots)
        free_logical_slot_infos(&cluster->dbarr.dbs[dbnum].slot_arr);

    get_logical_slot_infos_per_db(cluster, &cluster->dbarr.dbs[dbnum]);

    if (log_opts.verbose)
    {
        pg_log(PG_VERBOSE, "Database: %s", cluster->dbarr.dbs[dbnum].db_name);
        print_slot_infos(&cluster->dbarr.dbs[dbnum].slot_arr);
    }
}

~

I expected it could be simplified further still by using some variables

SUGGESTION 2:

if (cluster == &old_cluster)
    pg_log(PG_VERBOSE, "\nsource databases:");
else
    pg_log(PG_VERBOSE, "\ntarget databases:");

for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
{
DbInfo *pDbInfo = &cluster->dbarr.dbs[dbnum];
    if (pDbInfo->slot_arr.slots)
        free_logical_slot_infos(&pDbInfo->slot_arr);

    get_logical_slot_infos_per_db(cluster, pDbInfo);

    if (log_opts.verbose)
    {
        pg_log(PG_VERBOSE, "Database: %s", pDbInfo->db_name);
        print_slot_infos(&pDbInfo->slot_arr);
    }
}

~~~

14. get_logical_slot_infos_per_db

+ char query[QUERY_ALLOC];
+
+ query[0] = '\0'; /* initialize query string to empty */
+
+ snprintf(query + strlen(query), sizeof(query) - strlen(query),
+ "SELECT slot_name, plugin, two_phase "
+ "FROM pg_catalog.pg_replication_slots "
+ "WHERE database = current_database() AND temporary = false "
+ "AND wal_status IN ('reserved', 'extended');");

I didn't understand the purpose of those calls to 'strlen(query)'
since the string was initialised to empty-string immediately above.

~~~

15.
+static void
+print_slot_infos(LogicalSlotInfoArr *slot_arr)
+{
+ int slotnum;
+
+ for (slotnum = 0; slotnum < slot_arr->nslots; slotnum++)
+ pg_log(PG_VERBOSE, "slotname: %s: plugin: %s: two_phase %d",
+    slot_arr->slots[slotnum].slotname,
+    slot_arr->slots[slotnum].plugin,
+    slot_arr->slots[slotnum].two_phase);
+}

IMO those colons don't make sense.

BEFORE
"slotname: %s: plugin: %s: two_phase %d"

SUGGESTION
"slotname: %s, plugin: %s, two_phase: %d"

======
src/bin/pg_upgrade/pg_upgrade.h

16. LogicalSlotInfo

+typedef struct
+{
+ char    *slotname; /* slot name */
+ char    *plugin; /* plugin */
+ bool two_phase; /* Can the slot decode 2PC? */
+} LogicalSlotInfo;

The RelInfo had a comment for the typedef struct, so I think the
LogicalSlotInfo struct also should have a comment.

~~~

17. DbInfo

  RelInfoArr rel_arr; /* array of all user relinfos */
+ LogicalSlotInfoArr slot_arr; /* array of all logicalslotinfos */
 } DbInfo;

Should the comment say "LogicalSlotInfo" instead of "logicalslotinfos"?

======
.../t/003_logical_replication_slots.pl

18. RESULTS

I run this by 'make check' in the src/bin/pg_upgrade folder.

For some reason, the test does not work for me. The results I get are:

# +++ tap check in src/bin/pg_upgrade +++
t/001_basic.pl ...................... ok
t/002_pg_upgrade.pl ................. ok
t/003_logical_replication_slots.pl .. 3/? # Tests were run but no plan
was declared and done_testing() was not seen.
t/003_logical_replication_slots.pl .. Dubious, test returned 29 (wstat
7424, 0x1d00)
All 4 subtests passed

Test Summary Report
-------------------
t/003_logical_replication_slots.pl (Wstat: 7424 Tests: 4 Failed: 0)
  Non-zero exit status: 29
  Parse errors: No plan found in TAP output
Files=3, Tests=27, 128 wallclock secs ( 0.04 usr  0.01 sys + 18.02
cusr  6.06 csys = 24.13 CPU)
Result: FAIL
make: *** [check] Error 1

~

And the log file
(tmp_check/log/003_logical_replication_slots_old_node.log) shows the
following ERROR:

2023-05-09 12:19:25.330 AEST [32572] 003_logical_replication_slots.pl
LOG:  statement: SELECT
pg_create_logical_replication_slot('test_slot', 'test_decoding',
false, true);
2023-05-09 12:19:25.331 AEST [32572] 003_logical_replication_slots.pl
ERROR:  could not access file "test_decoding": No such file or
directory
2023-05-09 12:19:25.331 AEST [32572] 003_logical_replication_slots.pl
STATEMENT:  SELECT pg_create_logical_replication_slot('test_slot',
'test_decoding', false, true);
2023-05-09 12:19:25.335 AEST [32564] LOG:  received immediate shutdown request
2023-05-09 12:19:25.337 AEST [32564] LOG:  database system is shut down

~

Is it a bug? Or, if I am doing something wrong please let me know how
to run the test.

~~~

19.
+# Clean up
+rmtree($new_node->data_dir . "/pg_upgrade_output.d");
+$new_node->append_conf('postgresql.conf', "wal_level = 'logical'");
+$new_node->append_conf('postgresql.conf', "max_replication_slots = 0");

I think the last 2 lines are not "clean up". They are preparations for
the subsequent test, so maybe they should be commented as such.

~~~

20.
+# Clean up
+rmtree($new_node->data_dir . "/pg_upgrade_output.d");
+$new_node->append_conf('postgresql.conf', "max_replication_slots = 10");

I think the last line is not "clean up". It is preparation for the
subsequent test, so maybe it should be commented as such.

------
Kind Regards,
Peter Smith.
Fujitsu Australia