Re: Skipping logical replication transactions on subscriber side

On Thu, Aug 19, 2021 at 2:06 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Aug 18, 2021 at 12:12 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Aug 18, 2021 at 3:15 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Aug 18, 2021 at 10:00 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > On Wed, Aug 18, 2021 at 12:02 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Wed, Aug 18, 2021 at 6:53 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > On Tue, Aug 17, 2021 at 2:35 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > > > >
> > > > > > > > It's right that we use "STREAM STOP" rather than "STREAM END" in many
> > > > > > > > places such as elog messages, a callback name, and source code
> > > > > > > > comments. As far as I have found there are two places where we’re
> > > > > > > > using "STREAM STOP": LOGICAL_REP_MSG_STREAM_END and a description in
> > > > > > > > doc/src/sgml/protocol.sgml. Isn't it better to fix these
> > > > > > > > inconsistencies in the first place? I think “STREAM STOP” would be
> > > > > > > > more appropriate.
> > > > > > > >
> > > > > > >
> > > > > > > I think keeping STREAM_END in the enum 'LOGICAL_REP_MSG_STREAM_END'
> > > > > > > seems to be a bit better because of the value 'E' we use for it.
> > > > > >
> > > > > > But I think we don't care about the actual value of
> > > > > > LOGICAL_REP_MSG_STREAM_END since we use the enum value rather than
> > > > > > 'E'?
> > > > > >
> > > > >
> > > > > True, but here we are trying to be consistent with other enum values
> > > > > where we try to use the first letter of the last word (which is E in
> > > > > this case). I can see there are other cases where we are not
> > > > > consistent so it won't be a big deal if we won't be consistent here. I
> > > > > am neutral on this one, so, if you feel using STREAM_STOP would be
> > > > > better from a code readability perspective then that is fine.
> > > >
> > > > In addition of a code readability, there is a description in the doc
> > > > that mentions "Stream End" but we describe "Stream Stop" in the later
> > > > description, which seems a bug in the doc to me:
> > > >
> > >
> > > Doc changes looks good to me. But, I have question for code change:
> > >
> > > --- a/src/include/replication/logicalproto.h
> > > +++ b/src/include/replication/logicalproto.h
> > > @@ -65,7 +65,7 @@ typedef enum LogicalRepMsgType
> > > LOGICAL_REP_MSG_COMMIT_PREPARED = 'K',
> > > LOGICAL_REP_MSG_ROLLBACK_PREPARED = 'r',
> > > LOGICAL_REP_MSG_STREAM_START = 'S',
> > > - LOGICAL_REP_MSG_STREAM_END = 'E',
> > > + LOGICAL_REP_MSG_STREAM_STOP = 'E',
> > > LOGICAL_REP_MSG_STREAM_COMMIT = 'c',
> > >
> > > As this is changing the enum name and if any extension (logical
> > > replication extension) has started using it then they would require a
> > > change. As this is the latest change in PG-14, so it might be okay but
> > > OTOH, as this is just a code readability change, shall we do it only
> > > for PG-15?
> >
> > I think that the doc changes could be backpatched to PG14 but I think
> > we should do the code change only for PG15.
> >
>
> Okay, done that way!

Thanks!

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/