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

Hi Kuroda-san, I haven't looked at this thread for a very long time so
to re-familiarize myself with it I read all the latest v16-0001 patch.

Here are a number of minor review comments I noted in passing:

======
Commit message

1.
For pg_dump this commit includes a new option called
"--logical-replication-slots-only".
This option can be used to dump logical replication slots. When this option is
specified, the slot_name, plugin, and two_phase parameters are extracted from
pg_replication_slots. An SQL file is then generated which executes
pg_create_logical_replication_slot() with the extracted parameters.

~

This part doesn't do the actual execution, so maybe slightly reword this.

BEFORE
An SQL file is then generated which executes
pg_create_logical_replication_slot() with the extracted parameters.

SUGGESTION
An SQL file that executes pg_create_logical_replication_slot() with
the extracted parameters is generated.

~~~

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. Note that 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.

~~~

Maybe "restores from the dump" can be described more?

BEFORE
...and restores from the dump.

SUGGESTION
...and restores the slots using the
pg_create_logical_replication_slots() statements that the dump
generated (see above).

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

3. help

+
+ /*
+ * The option --logical-replication-slots-only is used only by pg_upgrade
+ * and should not be called by users, which is why it is not listed.
+ */
  printf(_("  --no-comments                do not dump comments\n"));
~

/not listed./not exposed by the help./

~~~

4. getLogicalReplicationSlots

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

PG16 is already in beta. I think this should now be changed to 170000, right?

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

5. check_new_cluster

+ /*
+ * Do additional works if --include-logical-replication-slots is required.
+ * These must be done before check_new_cluster_is_empty() because the
+ * slot_arr attribute of the new_cluster will be checked in the function.
+ */

SUGGESTION (minor rewording/grammar)
Do additional work if --include-logical-replication-slots was
specified. This must be done before check_new_cluster_is_empty()
because the slot_arr attribute of the new_cluster will be checked in
that function.

~~~

6. check_new_cluster_is_empty

+ /*
+ * If --include-logical-replication-slots is required, check the
+ * existence of slots.
+ */
+ if (user_opts.include_logical_slots)
+ {
+ LogicalSlotInfoArr *slot_arr = &new_cluster.dbarr.dbs[dbnum].slot_arr;
+
+ /* if nslots > 0, report just first entry and exit */
+ if (slot_arr->nslots)
+ pg_fatal("New cluster database \"%s\" is not empty: found logical
replication slot \"%s\"",
+ new_cluster.dbarr.dbs[dbnum].db_name,
+ slot_arr->slots[0].slotname);
+ }
+

6a.
There are a number of places in this function using
"new_cluster.dbarr.dbs[dbnum].XXX"

It is OK but maybe it would be tidier to up-front assign a local
variable for this?

DbInfo *pDbInfo = &new_cluster.dbarr.dbs[dbnum];

~

6b.
The above code adds an unnecessary blank line in the loop that was not
there previously.

~~~

7. check_for_parameter_settings

+/*
+ * Verify parameter settings for creating logical replication slots
+ */
+static void
+check_for_parameter_settings(ClusterInfo *new_cluster)

7a.
I felt this might have some missing words so it was meant to say:

SUGGESTION
Verify the parameter settings necessary for creating logical replication slots.

~

7b.
Maybe you can give this function a better name because there is no
hint in this generic name that it has anything to do with replication
slots.

~~~

8.
+ /* --include-logical-replication-slots can be used since PG16. */
+ if (GET_MAJOR_VERSION(new_cluster->major_version) <= 1500)
+ return;

PG16 is already in beta, so the version number (1500) and the comment
mentioning PG16 are outdated aren't they?

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

9.
 static void print_rel_infos(RelInfoArr *rel_arr);
-
+static void print_slot_infos(LogicalSlotInfoArr *slot_arr);

The removal of the existing blank line seems not a necessary part of this patch.

~~~

10. get_logical_slot_infos_per_db

+ char query[QUERY_ALLOC];
+
+ query[0] = '\0'; /* initialize query string to empty */
+
+ snprintf(query, sizeof(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');");

Does the initial assignment query[0] = '\0'; acheive anything? IIUC,
the next statement is simply going to overwrite that anyway.

~~~

11. free_db_and_rel_infos

+
+ /*
+ * db_arr has an additional attribute, LogicalSlotInfoArr slot_arr,
+ * but there is no need to free it. It has a valid member only when
+ * the cluster had logical replication slots in the previous call.
+ * However, in this case, a FATAL error is thrown, and we cannot reach
+ * this point.
+ */

Maybe this comment can be reworded? For example, the meaning of "in
the previous call" is not very clear. What previous call?

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

12. main

+ /*
+ * Create logical replication slots if requested.
+ *
+ * Note: This must be done after doing pg_resetwal command because the
+ * command will remove required WALs.
+ */
+ if (user_opts.include_logical_slots)
+ {
+ start_postmaster(&new_cluster, true);
+ create_logical_replication_slots();
+ stop_postmaster(false);
+ }

IMO "the command" is a bit vague. It might be better to be explicit
and say "... because pg_resetwal would remove XXXXX..."

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

13.
+typedef struct
+{
+ LogicalSlotInfo *slots;
+ int nslots;
+} LogicalSlotInfoArr;
+

I assume you mimicked the RelInfoArr struct, but IMO it makes more
sense for the field 'nslots' to come before the 'slots'.

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