Re: Failed transaction statistics to measure the logical replication progress

On Wed, Jan 12, 2022 at 6:04 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Thursday, December 23, 2021 6:37 PM Wang, Wei/王 威 <wangw(dot)fnst(at)fujitsu(dot)com> wrote:
> > On Wednesday, December 22, 2021 10:30 PM osumi(dot)takamichi(at)fujitsu(dot)com
> > <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > > On Wednesday, December 22, 2021 8:38 PM I wrote:
> > > > Do we expect these commit counts which come from empty transactions ?
> > > This is another issue discussed in [1] where the patch in the thread
> > > is a work in progress, I think.
> > > ......
> > > IMHO, the conclusion is we are currently in the middle of fixing the behavior.
> >
> > Thank you for telling me this.
> > After applying v19-* and
> > v15-0001-Skip-empty-transactions-for-logical-replication.patch,
> > I retested v19-* patches. The result of previous case looks good to me.
> >
> > But the results of following cases are also similar to previous unexpected result
> > which increases commit_count or abort_count unexpectedly.
> > [1]
> > (Based on environment in the previous example, set TWO_PHASE=true)
> > [Publisher] begin; insert into replica_test1 values(1,'1'); prepare transaction
> > 'id'; commit prepared 'id';
> >
> > In subscriber side, the commit_count of two records(sub1 and sub2) is
> > increased.
> >
> > [2]
> > (Based on environment in the previous example, set STREAMING=on)
> > [Publisher] begin; INSERT INTO replica_test1 SELECT i, md5(i::text) FROM
> > generate_series(1, 5000) s(i); commit;
> >
> > In subscriber side, the commit_count of two records(sub1 and sub2) is
> > increased.
> >
> > [3]
> > (Based on environment in the previous example, set TWO_PHASE=true)
> > [Publisher] begin; insert into replica_test1 values(1,'1'); prepare transaction
> > 'id'; rollback prepared 'id';
> >
> > In subscriber side, the abort_count of two records(sub1 and sub2) is
> > increased.
> >
> > I think the problem maybe is the patch you mentioned
> > (Skip-empty-transactions-for-logical-replication.patch) is not finished yet.
> > Share this information here.
> Hi, thank you for your report.
>
> Yes. As the patch's commit message mentions, the patch doesn't
> cover streaming and two phase transactions.
>
> In the above reply, I said that this was an independent issue
> and we were in the middle of the modification of the behavior,
> but empty transaction turned out to be harmful enough for this feature.
>

Isn't that because of this patch? I mean the patch is reporting count
even when during apply we haven't started any transaction. In
particular, if you would have reported stats from
apply_handle_commit_internal() when IsTransactionState() returns true,
then it shouldn't have updated the stats for an empty transaction.
Similarly, I see for rollback_prepared, you don't need to report stats
if there is no prepared transaction to rollback. I think for
commit_prepare case, we can't do much for empty xacts but why would
that be a problem of this patch? I think as far as this patch goes, it
reports the number of completed xacts (committed/aborted) in the
subscription worker, so, it doesn't need to handle any special cases
like empty transactions.

--
With Regards,
Amit Kapila.