Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Wed, Sep 2, 2020 at 10:55 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Sep 1, 2020 at 8:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > I have fixed all the comments except
> ..
> > 3. +# Change the local values of the extra columns on the subscriber,
> > +# update publisher, and check that subscriber retains the expected
> > +# values
> > +$node_subscriber->safe_psql('postgres', "UPDATE test_tab SET c =
> > 'epoch'::timestamptz + 987654321 * interval '1s'");
> > +$node_publisher->safe_psql('postgres', "UPDATE test_tab SET b = md5(a::text)");
> > +
> > +wait_for_caught_up($node_publisher, $appname);
> > +
> > +$result =
> > + $node_subscriber->safe_psql('postgres', "SELECT count(*),
> > count(extract(epoch from c) = 987654321), count(d = 999) FROM
> > test_tab");
> > +is($result, qq(3334|3334|3334), 'check extra columns contain locally
> > changed data');
> >
> > Again, how this test is relevant to streaming mode?
> >
>
> I think we can keep this test in one of the newly added tests say in
> 015_stream_simple.pl to ensure that after streaming transaction, the
> non-streaming one behaves expectedly. So we can change the comment as
> "Change the local values of the extra columns on the subscriber,
> update publisher, and check that subscriber retains the expected
> values. This is to ensure that non-streaming transactions behave
> properly after a streaming transaction."
>
> We can remove this test from the other two places
> 016_stream_subxact.pl and 020_stream_binary.pl.
>
> > 4. Apart from the above, I think we should think of minimizing the
> > test cases which can be committed with the base patch. We can later
> > add more tests.
> >
>
> We can combine the tests in 015_stream_simple.pl and
> 020_stream_binary.pl as I can't see a good reason to keep them
> separate. Then, I think we can keep only this part with the main patch
> and extract other tests into a separate patch. Basically, we can
> commit the basic tests with the main patch and then keep the advanced
> tests separately. I am afraid that there are some tests that don't add
> much value so we can review them separately.

Fixed

> One minor comment for option 'streaming = on', spacing-wise it should
> be consistent in all the tests.
>
> Similarly, we can combine 017_stream_ddl.pl and 021_stream_schema.pl
> as both contains similar tests. As per the above suggestion, this will
> be in a separate patch though.
>
> If you agree with the above suggestions then kindly make these
> adjustments and send the updated patch.

Done that way.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com