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

On Thu, Dec 22, 2022 at 9:48 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> On Thu, Dec 22, 2022 5:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Dec 22, 2022 at 1:15 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > >
> > > At Thu, 22 Dec 2022 12:35:46 +0530, Amit Kapila
> > <amit.kapila16@gmail.com> wrote in
> > > > I have addressed these comments in the attached. Additionally, I have
> > > > modified the docs and commit messages to make those clear. I think
> > > > instead of adding new tests with this patch, it may be better to
> > > > change some of the existing tests related to streaming to use this
> > > > parameter as that will clearly show one of the purposes of this patch.
> > >
> > > Being late but I'm warried that we might sometime regret that the lack
> > > of the explicit default. Don't we really need it?
> > >
> >
> > For this, I like your proposal for "buffered" as an explicit default value.
> >
> > > +        Allows streaming or serializing changes immediately in logical
> > decoding.
> > > +        The allowed values of <varname>logical_decoding_mode</varname>
> > are the
> > > +        empty string and <literal>immediate</literal>. When set to
> > > +        <literal>immediate</literal>, stream each change if
> > > +        <literal>streaming</literal> option is enabled, otherwise, serialize
> > > +        each change.  When set to an empty string, which is the default,
> > > +        decoding will stream or serialize changes when
> > > +        <varname>logical_decoding_work_mem</varname> is reached.
> > >
> > > With (really) fresh eyes, I took a bit long time to understand what
> > > the "streaming" option is. Couldn't we augment the description by a
> > > bit?
> > >
> >
> > Okay, how about modifying it to: "When set to
> > <literal>immediate</literal>, stream each change if
> > <literal>streaming</literal> option (see optional parameters set by
> > CREATE SUBSCRIPTION) is enabled, otherwise, serialize each change.
> >
>
> I updated the patch to use "buffered" as the explicit default value, and include
> Amit's changes about document.
>
> Besides, I tried to reduce data size in streaming subscription tap tests by this
> new GUC (see 0002 patch). But I didn't covert all streaming tap tests because I
> think we also need to cover the case that there are lots of changes. So, 015* is
> not modified.

If we want to eventually convert 015 some time, isn't it better to
include it even if it requires many changes? Is there any reason we
want to change 017 in a separate patch?

> And 017* is not modified because streaming transactions and
> non-streaming transactions are tested alternately in this test.

How about 029_on_error.pl? It also sets logical_decoding_work_mem to
64kb to test the STREAM COMMIT case.

>
> I collected the time to run these tests before and after applying the patch set
> on my machine. In debug version, it saves about 5.3 s; and in release version,
> it saves about 1.8 s. The time of each test is attached.

Nice improvements.

Regards,

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