On Tue, Feb 20, 2024 at 12:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Feb 20, 2024 at 8:25 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Some comments not related to the patch but to the existing code:
> >
> > ---
> > It might have already been discussed but is the
> > src/backend/replication/logical the right place for the slocsync.c? If
> > it's independent of logical decoding/replication, is under
> > src/backend/replication could be more appropriate?
> >
Thank you for the comment.
>
> This point has not been discussed, so thanks for raising it. I think
> the reasoning behind keeping it in logical is that this file contains
> a code for logical slot syncing and a worker doing that. As it is
> mostly about logical slot syncing so there is an argument to keep it
> under logical. In the future, we may need to extend this functionality
> to have a per-db slot sync worker as well in which case it will
> probably be again somewhat related to logical slots.
That's a valid argument.
> Having said that,
> there is an argument to keep it under replication as well because the
> functionality it provides is for physical standbys.
Another argument to keep it under replication is; all files under the
replication/logical directory are logical decoding and logical
replication infrastructures. IOW these are the functionality built on
top of (logical) replication slots. On the other hand, the slotsync
worker (and slotsync functionality) looks a part of slot management
functionality, which seems the same layer of slot.c.
BTW the slotsync.c of v91 patch includes "replication/logical.h" but
it isn't actually necessary and #include'ing "replication/slot.h" is
sufficient.
Regards
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com