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

Hi Kuroda-san. I checked again the v11-0001.

Here are a few more review comments.

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

1. help

  printf(_("  --inserts                    dump data as INSERT
commands, rather than COPY\n"));
  printf(_("  --load-via-partition-root    load partitions via the
root table\n"));
+ printf(_("  --logical-replication-slots-only\n"
+ "                               dump only logical replication slots,
no schema or data\n"));
  printf(_("  --no-comments                do not dump comments\n"));

Now you removed the PG Docs for the internal pg_dump option based on
my previous review comment (see [2]#1). So does it mean this "help"
also be removed so this option will be completely invisible to the
user? I am not sure, but if you do choose to remove this help then
probably a comment should be added here to explain why it is
deliberately not listed.

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

2. check_new_cluster

Although you wrote "Added", I don't think my previous comment ([1]#8)
was yet addressed.

What I mean to say ask was: can that call to get_logical_slot_infos()
be done later, only when you know that option was specified?

e.g

BEFORE
get_logical_slot_infos(&new_cluster);
...
if (user_opts.include_logical_slots)
    check_for_parameter_settings(&new_cluster);

SUGGESTION
if (user_opts.include_logical_slots)
{
    get_logical_slot_infos(&new_cluster);
    check_for_parameter_settings(&new_cluster);
}

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

3. get_db_and_rel_infos

> src/bin/pg_upgrade/info.c
>
> 11. get_db_and_rel_infos
>
> + {
>   get_rel_infos(cluster, &cluster->dbarr.dbs[dbnum]);
>
> + /*
> + * Additionally, slot_arr must be initialized because they will be
> + * checked later.
> + */
> + cluster->dbarr.dbs[dbnum].slot_arr.nslots = 0;
> + cluster->dbarr.dbs[dbnum].slot_arr.slots = NULL;
> + }
>
> 11a.
> I think probably it would have been easier to just use 'pg_malloc0'
> instead of 'pg_malloc' in the get_db_infos, then this code would not
> be necessary.
I was not sure whether it is OK to change like that because of the
performance efficiency. But OK, fixed.
> 11b.
> BTW, shouldn't this function also be calling free_logical_slot_infos()
> too? That will also have the same effect (initializing the slot_arr)
> but without having to change anything else.

~

Above is your reply ([2]11a). If you were not sure about the malloc0
then I think the suggestion ([1]#12b) achieves the same thing and
initializes those fields. You did not reply to 12b, so I wondered if
you accidentally missed that point.

~~~

4. get_logical_slot_infos

+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ DbInfo *pDbInfo = &cluster->dbarr.dbs[dbnum];
+
+ if (pDbInfo->slot_arr.slots)
+ free_logical_slot_infos(&pDbInfo->slot_arr);

Maybe it is ok, but it seems unusual that this
get_logical_slot_infos() is also doing a free. I didn't notice this
same pattern with the other get_XXX functions. Why is it needed? Even
if pDbInfo->slot_arr.slots was not NULL, is the information stale or
will you just end up re-fetching the same info?

======
.../pg_upgrade/t/003_logical_replication_slots.pl

5.
+# Preparations for the subsequent test. The case max_replication_slots is set
+# to 0 is prohibit.

/prohibit/prohibited/

------
[1] My v10 review -
https://www.postgresql.org/message-id/CAHut%2BPtpQaKVfqr-8KUtGZqei1C9gWF0%2BY8n1UafvAQeS4G_hg%40mail.gmail.com
[2] Kuroda-san's reply to my v10 review -
https://www.postgresql.org/message-id/TYAPR01MB5866A537AC9AD46E49345A78F5769%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia