On Wed, Dec 7, 2022 at 12:55 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Dec 7, 2022 at 7:31 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Wed, Dec 7, 2022 at 8:46 AM Peter Smith <smithpb2...@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