On Thu, Nov 25, 2021 at 10:06 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thur, Nov 25, 2021 8:29 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > On Thu, Nov 25, 2021 at 1:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Nov 24, 2021 at 5:14 PM Masahiko Sawada
> > <sawada.mshk@gmail.com> wrote:
> > > >
> > > > Changed. I've removed first_error_time as per discussion on the
> > > > thread for adding xact stats.
> > > >
> > >
> > > We also agreed to change the column names to start with last_error_*
> > > [1]. Is there a reason to not make those changes? Do you think that we
> > > can change it just before committing that patch? I thought it might be
> > > better to do it that way now itself.
> >
> > Oh, I thought that you think that we change the column names when adding xact
> > stats to the view. But these names also make sense even without the xact stats.
> > I've attached an updated patch. It also incorporated comments from Vignesh
> > and Greg.
> >
> Hi,
>
> I only noticed some minor things in the testcases
>
> 1)
> +$node_publisher->append_conf('postgresql.conf',
> + qq[
> +logical_decoding_work_mem = 64kB
> +]);
>
> It seems we don’t need set the decode_work_mem since we don't test streaming ?
>
> 2)
> +$node_publisher->safe_psql('postgres',
> + q[
> +CREATE PUBLICATION tap_pub FOR TABLE test_tab1, test_tab2;
> +]);
>
> There are a few places where only one command exists in the 'q[' or 'qq[' like the above code.
> To be consistent, I think it might be better to remove the wrap here, maybe we can write like:
> $node_publisher->safe_psql('postgres',
> ' CREATE PUBLICATION tap_pub FOR TABLE test_tab1, test_tab2;');
>
Indeed. Attached an updated patch. Thanks!
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/