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

Dear Vignesh,

Thanks for reviewing! You can available new version in [1].

>
> Few comments:
> 1) Should we add binary upgrade check "CHECK_IS_BINARY_UPGRADE" for
> this funcion too:
> +binary_upgrade_create_logical_replication_slot(PG_FUNCTION_ARGS)
> +{
> + Name name = PG_GETARG_NAME(0);
> + Name plugin = PG_GETARG_NAME(1);
> +
> + /* Temporary slots is never handled in this function */
> + bool two_phase = PG_GETARG_BOOL(2);

Yeah, needed. For testing purpose I did not add, but it should have.
Added.

> 2) Generally we are specifying the slot name in this case, is slot
> name null check required:
> +Datum
> +binary_upgrade_validate_wal_logical_end(PG_FUNCTION_ARGS)
> +{
> + Name slot_name;
> + XLogRecPtr end_of_wal;
> + LogicalDecodingContext *ctx = NULL;
> + bool has_record;
> +
> + CHECK_IS_BINARY_UPGRADE;
> +
> + /* Quick exit if the input is NULL */
> + if (PG_ARGISNULL(0))
> + PG_RETURN_BOOL(false);

NULL check was added. I felt that we should raise an ERROR.

> 3) Since this is similar to pg_create_logical_replication_slot, can we
> add a comment saying any change in pg_create_logical_replication_slot
> would also need the same check to be added in
> binary_upgrade_create_logical_replication_slot:
> +/*
> + * SQL function for creating a new logical replication slot.
> + *
> + * This function is almost same as pg_create_logical_replication_slot(), but
> + * this can specify the restart_lsn.
> + */
> +Datum
> +binary_upgrade_create_logical_replication_slot(PG_FUNCTION_ARGS)
> +{
> + Name name = PG_GETARG_NAME(0);
> + Name plugin = PG_GETARG_NAME(1);
> +
> + /* Temporary slots is never handled in this function */

Added.

> 4) Any conclusion on this try catch comment, do you want to add which
> setting you want to revert in catch, if try/catch is not required we
> can remove this comment:
> + ReplicationSlotAcquire(NameStr(*slot_name), true);
> +
> + /* XXX: Is PG_TRY/CATCH needed around here? */
> +
> + /*
> + * We use silent mode here to decode all changes without
> outputting them,
> + * allowing us to detect all the records that could be sent downstream.
> + */

After considering more, it's OK to raise an ERROR because caller can detect it.
Also, there are any setting to be reverted. The comment is removed.

> 5) I felt these 2 comments can be combined as both are trying to say
> the same thing:
> + * This is a special purpose function to ensure that there are no WAL records
> + * pending to be decoded after the given LSN.
> + *
> + * It is used to ensure that there is no pending WAL to be consumed for
> + * the logical slots.

Later part was removed.

> 6) I feel this memset is not required as we are initializing at the
> beginning of function, if you want to keep the memset, the
> initialization can be removed:
> + values[2] = CStringGetTextDatum(xlogfilename);
> +
> + memset(nulls, 0, sizeof(nulls));
> +
> + tuple = heap_form_tuple(tupdesc, values, nulls);

The initialization was removed to follow pg_create_logical_replication_slot.

> 7) looks like a typo, "mu" should be "must":
> + /*
> + * Also, we mu execute pg_log_standby_snapshot() when logical
> replication
> + * slots are migrated. Because RUNNING_XACTS record is
> required to create
> + * a consistent snapshot.
> + */
> + if (count_old_cluster_logical_slots())
> + create_consistent_snapshot();

Fixed.

> 8) consitent should be consistent:
> +/*
> + * Log the details of the current snapshot to the WAL, allowing the snapshot
> + * state to be reconstructed for logical decoding on the upgraded slots.
> + */
> +static void
> +create_consistent_snapshot(void)
> +{
> + DbInfo *old_db = &old_cluster.dbarr.dbs[0];
> + PGconn *conn;
> +
> + prep_status("Creating a consitent snapshot on new cluster");

Fixed.

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866068CB6591C8AE1F9690BF5CDA%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED