RE: Skipping logical replication transactions on subscriber side

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;');

The others LGTM.

Best regards,
Hou zj