Re: Synchronizing slots from primary to standby

On Wed, Jan 17, 2024 at 3:08 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Tue, Jan 16, 2024 at 05:27:05PM +0530, shveta malik wrote:
> > PFA v62. Details:
>
> Thanks!
>
> > v62-003:
> > It is a new patch which attempts to implement slot-sync worker as a
> > special process which is neither a bgworker nor an Auxiliary process.
> > Here we get the benefit of converting enable_syncslot to a PGC_SIGHUP
> > Guc rather than PGC_POSTMASTER. We launch the slot-sync worker only if
> > it is hot-standby and 'enable_syncslot' is ON.
>
> The implementation looks reasonable to me (from what I can see some parts is
> copy/paste from an already existing "special" process and some parts are
> "sync slot" specific) which makes fully sense.
>
> A few remarks:
>
> 1 ===
> +                * Was it the slot sycn worker?
>
> Typo: sycn
>
> 2 ===
> +                * ones), and no walwriter, autovac launcher or bgwriter or slot sync
>
> Instead? "* ones), and no walwriter, autovac launcher, bgwriter or slot sync"
>
> 3 ===
> + * restarting slot slyc worker. If stopSignaled is set, the worker will
>
> Typo: slyc
>
> 4 ===
> +/* Flag to tell if we are in an slot sync worker process */
>
> s/an/a/ ?
>
> 5 === (coming from v62-0002)
> +       Assert(tuplestore_tuple_count(res->tuplestore) == 1);
>
> Is it even possible for the related query to not return only one row? (I think the
> "count" ensures it).
>
> 6 ===
>         if (conninfo_changed ||
>                 primary_slotname_changed ||
> +               old_enable_syncslot != enable_syncslot ||
>                 (old_hot_standby_feedback != hot_standby_feedback))
>         {
>                 ereport(LOG,
>                                 errmsg("slot sync worker will restart because of"
>                                            " a parameter change"));
>
> I don't think "slot sync worker will restart" is true if one change enable_syncslot
> from on to off.
>
> IMHO, v62-003 is in good shape and could be merged in v62-002 (that would ease
> the review). But let's wait to see if others think differently.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com


PFA v63.

--It addresses comments by Peter given in [1], [2], comment by Nisha
given in [3], comments by Bertrand given in [4]
--It also moves race-condition fix from patch003 to patch002 as
suggested by Swada-san offlist. Race-condition is mentioned in [5]

All the changes are in patch02, patch003 and patch006.

[1]: https://www.postgresql.org/message-id/CAHut%2BPuECB8fNBfXMdTHSMKF9kL%3D0XqPw1Am4NVahfJSSHzoYg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAHut%2BPt0uum%2B6Hg5UDofWMEJWhVEyArM1b0_B94UJmRcQmz7DA%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CABdArM73qdHyA0nteDLAQrfKNHRP%2B5Qq6p8uobg5bkE3EWiC%2Bg%40mail.gmail.com
[4]: https://www.postgresql.org/message-id/ZaegJe9JpUiQeV%2BD%40ip-10-97-1-34.eu-west-3.compute.internal
[5]: https://www.postgresql.org/message-id/CAD21AoA5izeKpp9Ei4Cd745pKX3wn-TRvhhmPFEW9UY1nx%2B_aw%40mail.gmail.com

thanks
Shveta