Re: [HACKERS] logical decoding of two-phase transactions

On Tue, Apr 27, 2021 at 1:41 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Apr 21, 2021 at 12:13 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Tue, Apr 20, 2021 at 3:45 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Please find attached the latest patch set v73`*
> > >
> > > Differences from v72* are:
> > >
> > > * Rebased to HEAD @ today (required because v72-0001 no longer applied cleanly)
> > >
> > > * Minor documentation correction for protocol messages for Commit Prepared ('K')
> > >
> > > * Non-functional code tidy (mostly proto.c) to reduce overloading
> > > different meanings to same member names for prepare/commit times.
> >
> >
> > Please find attached a re-posting of patch set v73*
>
> Few comments when I was having a look at the tests added:

Thanks for your feedback comments. My replies are inline below.

> 1) Can the below:
> +# check inserts are visible. 22 should be rolled back. 21 should be committed.
> +$result = $node_subscriber->safe_psql('postgres', "SELECT count(*)
> FROM tab_full where a IN (21);");
> +is($result, qq(1), 'Rows committed are on the subscriber');
> +$result = $node_subscriber->safe_psql('postgres', "SELECT count(*)
> FROM tab_full where a IN (22);");
> +is($result, qq(0), 'Rows rolled back are not on the subscriber');
>
> be changed to:
> $result = $node_subscriber->safe_psql('postgres', "SELECT a FROM
> tab_full where a IN (21,22);");
> is($result, qq(21), 'Rows committed are on the subscriber');
>
> And Test count need to be reduced to "use Test::More tests => 19"
>
OK. Updated in v74.

> 2) we can change tx to transaction:
> +# check the tx state is prepared on subscriber(s)
> +$result = $node_B->safe_psql('postgres', "SELECT count(*) FROM
> pg_prepared_xacts;");
> +is($result, qq(1), 'transaction is prepared on subscriber B');
> +$result = $node_C->safe_psql('postgres', "SELECT count(*) FROM
> pg_prepared_xacts;");
> +is($result, qq(1), 'transaction is prepared on subscriber C');
>
OK. Updated in v74

> 3) There are few more instances present in the same file, those also
> can be changed.
OK. I found no others in the same file, but there were similar cases
in the 021 TAP test. Those were also updated in v74/

>
> 4) Can the below:
>  check inserts are visible at subscriber(s).
> # 22 should be rolled back.
> # 21 should be committed.
> $result = $node_B->safe_psql('postgres', "SELECT count(*) FROM
> tab_full where a IN (21);");
> is($result, qq(1), 'Rows committed are present on subscriber B');
> $result = $node_B->safe_psql('postgres', "SELECT count(*) FROM
> tab_full where a IN (22);");
> is($result, qq(0), 'Rows rolled back are not present on subscriber B');
> $result = $node_C->safe_psql('postgres', "SELECT count(*) FROM
> tab_full where a IN (21);");
> is($result, qq(1), 'Rows committed are present on subscriber C');
> $result = $node_C->safe_psql('postgres', "SELECT count(*) FROM
> tab_full where a IN (22);");
> is($result, qq(0), 'Rows rolled back are not present on subscriber C');
>
> be changed to:
> $result = $node_B->safe_psql('postgres', "SELECT a FROM tab_full where
> a IN (21,22);");
> is($result, qq(21), 'Rows committed are on the subscriber');
> $result = $node_C->safe_psql('postgres', "SELECT a FROM tab_full where
> a IN (21,22);");
> is($result, qq(21), 'Rows committed are on the subscriber');
>
> And Test count need to be reduced to "use Test::More tests => 27"
>
OK. Updated in v74.

> 5) should we change "Two phase commit" to "Two phase commit state" :
> +               /*
> +                * Binary, streaming, and two_phase are only supported
> in v14 and
> +                * higher
> +                */
>                 if (pset.sversion >= 140000)
>                         appendPQExpBuffer(&buf,
>                                                           ", subbinary
> AS \"%s\"\n"
> -                                                         ", substream
> AS \"%s\"\n",
> +                                                         ", substream
> AS \"%s\"\n"
> +                                                         ",
> subtwophasestate AS \"%s\"\n",
>
> gettext_noop("Binary"),
> -
> gettext_noop("Streaming"));
> +
> gettext_noop("Streaming"),
> +
> gettext_noop("Two phase commit"));
>
Not updated. I think the column name is already the longest one and
this just makes it even longer - far too long IMO. I am not sure what
is better having the “state” suffix. After all, booleans are also
states. Anyway, I did not make this change now but if people feel
strongly about it then I can revisit it.

------
Kind Regards,
Peter Smith.
Fujitsu Australia