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

Dear Peter,

Thank you for reviewing! PSA new version patchset.

> ======
> 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.

Changed.

> 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).

Fixed.

> 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./

Fixed.

> 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?

That's right, fixed.

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

Fixed.

> 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];

Seems better, fixed.

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

Removed.

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

Changed.

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

Renamed to check_for_logical_replication_slots(), how do you think?

> 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?

Right, fixed.

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

Added.

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

This was garbage of previous versions. Removed.

> 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?

After considering more, I thought it should be more simpler. What I wanted to say
was that the slot_arr.slots did not have malloc'd memory. So I added Assert() for
the confirmation and changed comments. For that purpose pg_malloc0() is also
introduced in get_db_infos(). How do you think?

> 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..."

Changed.

> 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'.

Yeah, I followed that, but no strong opinion. Fixed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED