From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Peter Smith' <smithpb2250(at)gmail(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 13:01:51 |
Message-ID: | TYAPR01MB5866C310960B2D35D97DAFA7F5E0A@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Dear Peter,
Thank you for reviewing!
> 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.
IIUC these points were disagreed by Amit, so I would keep my code until he posts
opinions.
> 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.
>
Oh, sorry for that. Both places was fixed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
From | Date | Subject | |
---|---|---|---|
Next Message | Konstantin Knizhnik | 2023-08-28 13:05:28 | Re: Support prepared statement invalidation when result types change |
Previous Message | Hayato Kuroda (Fujitsu) | 2023-08-28 13:01:47 | RE: [PoC] pg_upgrade: allow to upgrade publisher node |