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

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