Re: Synchronizing slots from primary to standby

On Wed, Jan 17, 2024 at 3:08 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)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.

Thanks for the feedback. I have addressed the comments in v63 except 5th one.

> 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).

I think you are right. This assertion was added sometime back on the
basis of feedback on hackers. Let me review that again. I can consider
this comment in the next version.

> 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.

Yes, right. I have changed the log-msg in this specific case.

>
> 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