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

Dear Vignesh,

Thank you for giving comments. New patchset can be available in [1].

> Thanks for the updated patch.
> A Few comments:
> 1) if the verbose option is enabled, we should print the new slot
> information, we could add a function print_slot_infos similar to
> print_rel_infos which could print slot name and two_phase is enabled
> or not.
> +       end_progress_output();
> +       check_ok();
> +
> +       /* update new_cluster info now that we have objects in the databases */
> +       get_db_and_rel_infos(&new_cluster);
> +}

I was not sure we should add the print because any other objects like publication
and subscription seem not to be printed, but added.
While implementing it, I thought that calling get_db_and_rel_infos() again
was not efficient because free_db_and_rel_infos() will be called at that time. So I added
get_logical_slot_infos() instead.
Additionally, I added a check for check_new_cluster_is_empty() for making ensure that
there are no logical slots on new node.

> 2) Since we will be using this option with pg_upgrade, should we use
> this along with the --binary-upgrade option only?
> +       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");

Right, I added the check.

> 3) Since it two_phase is boolean, can we use bool data type instead of string:
> +               slotinfo[i].dobj.objType = DO_LOGICAL_REPLICATION_SLOT;
> +
> +               slotinfo[i].dobj.catId.tableoid = InvalidOid;
> +               slotinfo[i].dobj.catId.oid = InvalidOid;
> +               AssignDumpId(&slotinfo[i].dobj);
> +
> +               slotinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i,
> i_slotname));
> +
> +               slotinfo[i].plugin = pg_strdup(PQgetvalue(res, i, i_plugin));
> +               slotinfo[i].twophase = pg_strdup(PQgetvalue(res, i,
> i_twophase));
> 
> We can change it to something like:
> if (strcmp(PQgetvalue(res, i, i_twophase), "t") == 0)
> slotinfo[i].twophase = true;
> else
> slotinfo[i].twophase = false;

Seems right, fixed.

> 4) The comments are inconsistent, some have termination characters and
> some don't. We can keep it consistent:
> +# Can be changed to test the other modes.
> +my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
> +
> +# Initialize old publisher node
> +my $old_publisher = PostgreSQL::Test::Cluster->new('old_publisher');
> +$old_publisher->init(allows_streaming => 'logical');
> +$old_publisher->start;
> +
> +# Initialize subscriber node
> +my $subscriber = PostgreSQL::Test::Cluster->new('subscriber');
> +$subscriber->init(allows_streaming => 'logical');
> +$subscriber->start;
> +
> +# Schema setup
> +$old_publisher->safe_psql('postgres',
> +       "CREATE TABLE tbl AS SELECT generate_series(1,10) AS a");
> +$subscriber->safe_psql('postgres', "CREATE TABLE tbl (a int)");
> +
> +# Initialize new publisher node

Removed all termination.

> 5) should we use free instead of pfree as used in other function like
> dumpForeignServer:
> +               appendPQExpBuffer(query, ");");
> +
> +               ArchiveEntry(fout, slotinfo->dobj.catId, slotinfo->dobj.dumpId,
> +                                        ARCHIVE_OPTS(.tag = slotname,
> +
> .description = "REPLICATION SLOT",
> +
> .section = SECTION_POST_DATA,
> +
> .createStmt = query->data));
> +
> +               pfree(slotname);
> +               destroyPQExpBuffer(query);
> +       }
> +}

Actually it works because for the client, the pfree() is just a wrapper of pg_free(),
but I agreed that it should be fixed. So did that.

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


Best Regards,
Hayato Kuroda
FUJITSU LIMITED