From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bruce Momjian <bruce(at)momjian(dot)us>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com> |
Subject: | Re: [PoC] pg_upgrade: allow to upgrade publisher node |
Date: | 2023-08-28 07:31:16 |
Message-ID: | CAHut+Pu3765fuTSrRUUhZtuRVQCU3NP+XYQ9Rrj=qXW6hAxOQw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi, here are my review comments for v26-0003
It seems I must defend some of my previous suggestions from v25* [1],
so here goes...
======
src/bin/pg_upgrade/check.c
1. check_and_dump_old_cluster
CURRENT CODE (with v26-0003 patch applied)
/* Extract a list of logical replication slots */
get_old_cluster_logical_slot_infos();
...
/*
* Logical replication slots can be migrated since PG17. See comments atop
* get_old_cluster_logical_slot_infos().
*/
if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
{
check_old_cluster_for_lost_slots();
/*
* Do additional checks if a live check is not required. This requires
* that confirmed_flush_lsn of all the slots is the same as the latest
* checkpoint location, but it would be satisfied only when the server
* has been shut down.
*/
if (!live_check)
check_old_cluster_for_confirmed_flush_lsn();
}
SUGGESTION
/*
* Logical replication slots can be migrated since PG17. See comments atop
* get_old_cluster_logical_slot_infos().
*/
if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700) // NOTE 1a.
{
/* Extract a list of logical replication slots */
get_old_cluster_logical_slot_infos();
if (count_old_cluster_slots()) // NOTE 1b.
{
check_old_cluster_for_lost_slots();
/*
* Do additional checks if a live check is not required. This requires
* that confirmed_flush_lsn of all the slots is the same as the latest
* checkpoint location, but it would be satisfied only when the server
* has been shut down.
*/
if (!live_check)
check_old_cluster_for_confirmed_flush_lsn();
}
}
~~
Benefits:
1a.
One version check instead of multiple.
~
1b.
Upfront slot counting means
- only call 1 time to count_old_cluster_slots().
- unnecessary calls to other check* functions are avoided
~
1c.
get_old_cluster_logical_slot_infos
- No version check is needed.
check_old_cluster_for_lost_slots
- Call to count_old_cluster_slots is not needed
- Quick exit not needed.
check_old_cluster_for_confirmed_flush_lsn
- Call to count_old_cluster_slots is not needed
- Quick exit not needed.
~~~
2. check_old_cluster_for_lost_slots
+ /* Quick exit if the cluster does not have logical slots. */
+ if (count_old_cluster_logical_slots() == 0)
+ return;
Refer [1]#4. Can remove this because #1b above.
~~~
3. check_old_cluster_for_confirmed_flush_lsn
+ /* Quick exit if the cluster does not have logical slots. */
+ if (count_old_cluster_logical_slots() == 0)
+ return;
Refer [1]#5. Can remove this because #1b above.
~~~
4. .../t/003_logical_replication_slots.pl
/shipped/replicated/
Kuroda-san 26/8 wrote:
You meant to say s/replicated/shipped/, right? Fixed.
No, I meant what I wrote for [1]#7. I was referring to the word
"shipped" in the message 'check changes are shipped to the
subscriber'. Now there are 2 places to change instead of one.
------
[1] my review of v25-0003.
https://www.postgresql.org/message-id/CAHut%2BPsdkhcVG5GY4ZW0DMUF8FG%3DWvjaGN%2BNA4XFLrzxWSQXVA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2023-08-28 08:04:45 | Re: Autogenerate some wait events code and documentation |
Previous Message | Peter Smith | 2023-08-28 07:27:27 | Re: [PoC] pg_upgrade: allow to upgrade publisher node |