Re: Force streaming every change in logical decoding - Mailing list pgsql-hackers

On Wed, Dec 7, 2022 at 12:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Dec 7, 2022 at 7:31 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Dec 7, 2022 at 8:46 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > >
> > > > Yeah, I think this can also help in reducing the time for various
> > > > tests in test_decoding/stream and
> > > > src/test/subscription/t/*_stream_*.pl file by reducing the number of
> > > > changes required to invoke streaming mode. Can we think of making this
> > > > GUC extendible to even test more options on server-side (publisher)
> > > > and client-side (subscriber) cases? For example, we can have something
> > > > like logical_replication_mode with the following valid values: (a)
> > > > server_serialize: this will serialize each change to file on
> > > > publishers and then on commit restore and send all changes; (b)
> > > > server_stream: this will stream each change as currently proposed in
> > > > your patch. Then if we want to extend it for subscriber-side testing
> > > > then we can introduce new options like client_serialize for the case
> > > > being discussed in the email [1].
> > > >
> > > > Thoughts?
> > >
> > > There is potential for lots of developer GUCs for testing/debugging in
> > > the area of logical replication but IMO it might be better to keep
> > > them all separated. Putting everything into a single
> > > 'logical_replication_mode' might cause difficulties later when/if you
> > > want combinations of the different modes.
> >
> > I think we want the developer option that forces streaming changes
> > during logical decoding to be PGC_USERSET but probably the developer
> > option for testing the parallel apply feature would be PGC_SIGHUP.
> >
>
> Ideally, that is true but if we want to combine the multiple modes in
> one parameter, is there a harm in keeping it as PGC_SIGHUP?

It's not a big harm but we will end up doing ALTER SYSTEM and
pg_reload_conf() even in regression tests (e.g. in
test_decoding/stream.sql).

>
> > Also, since streaming changes is not specific to logical replication
> > but to logical decoding, I'm not sure logical_replication_XXX is a
> > good name. IMO having force_stream_mode and a different GUC for
> > testing the parallel apply feature makes sense to me.
> >
>
> But if we want to have a separate variable for testing/debugging
> streaming like force_stream_mode, why not for serializing as well? And
> if we want for both then we can even think of combining them in one
> variable as logical_decoding_mode with values as 'stream' and
> 'serialize'.

Making it enum makes sense to me.

> The first one specified would be given preference. Also,
> the name force_stream_mode doesn't seem to convey that it is for
> logical decoding.

Agreed.

> On one side having separate GUCs for publisher and subscriber seems to
> give better flexibility but having more GUCs also sometimes makes them
> less usable. Here, my thought was to have a single or as few GUCs as
> possible which can be extendible by providing multiple values instead
> of having different GUCs. I was trying to map this with the existing
> string parameters in developer options.

I see your point. On the other hand, I'm not sure it's a good idea to
control different features by one GUC in general. The developer option
could be an exception?

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com