On Thur, Nov 25, 2021 8:29 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> On Thu, Nov 25, 2021 at 1:57 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Wed, Nov 24, 2021 at 5:14 PM Masahiko Sawada
> <sawada.m...@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;');
The others LGTM.
Best regards,
Hou zj
