RE: Failed transaction statistics to measure the logical replication progress - Mailing list pgsql-hackers

On Wednesday, November 17, 2021 10:00 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Nov 17, 2021 at 9:44 AM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > On Wednesday, November 17, 2021 12:19 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > > On Tue, Nov 16, 2021 at 9:34 PM osumi.takamichi@fujitsu.com
> > > <osumi.takamichi@fujitsu.com> wrote:
> > > >
> > > > On Monday, November 15, 2021 9:14 PM I wrote:
> > > > > I've conducted some update for this.
> > > > > (The rebased part is only C code and checked by pgindent)
> > > > I'll update my patches since a new skip xid patch has been shared
> > > > in [1].
> > > >
> > > > This version includes some minor renames of functions that are
> > > > related to transaction sizes.
> > >
> > > I've looked at v12-0001 patch. Here are some comments:
> > Thank you for paying attention to this thread !
> >
> >
> > > -   TupleDescInitEntry(tupdesc, (AttrNumber) 3, "relid",
> > > +   TupleDescInitEntry(tupdesc, (AttrNumber) 3, "last_error_relid",
> > >                        OIDOID, -1, 0);
> > > -   TupleDescInitEntry(tupdesc, (AttrNumber) 4, "command",
> > > +   TupleDescInitEntry(tupdesc, (AttrNumber) 4,
> > > + "last_error_command",
> > >                        TEXTOID, -1, 0);
> > > -   TupleDescInitEntry(tupdesc, (AttrNumber) 5, "xid",
> > > +   TupleDescInitEntry(tupdesc, (AttrNumber) 5, "last_error_xid",
> > >                        XIDOID, -1, 0);
> > > -   TupleDescInitEntry(tupdesc, (AttrNumber) 6, "error_count",
> > > +   TupleDescInitEntry(tupdesc, (AttrNumber) 6, "last_error_count",
> > >                        INT8OID, -1, 0);
> > > -   TupleDescInitEntry(tupdesc, (AttrNumber) 7, "error_message",
> > > +   TupleDescInitEntry(tupdesc, (AttrNumber) 7,
> > > + "last_error_message",
> > >
> > > If renaming column names clarifies those meanings, the above changes
> > > should be included into my patch that introduces
> > > pg_stat_subscription_workers view?
> 
> Right.
> 
> > At first, your column names of pg_stat_subscription_workers look
> > totally OK to me by itself and I thought I should take care of those renaming at
> the commit timing of my stats patches.
> >
> 
> Can you please tell us why you think the names in your proposed patch are
> better than the existing names proposed in Sawada-San's patch? Is it because
> those fields always contain the information of the last or latest error that
> occurred in the corresponding subscription worker?
This is one reason. 

Another big reason comes from the final alignment when we list up all columns of both patches.
The patches in this thread is trying to introduce a column that indicates
cumulative count of error to show all error counts that the worker got in the past.
In this thread, after two or three improvements, this column name has reached to
a simple one 'error_count' (aligned with 'commit_count' and 'abort_count').
Then we need to differentiate what this thread's patch is trying to introduce and
what skip xid patch is introducing.

> If so, I am not very sure if that is a good reason to increase the length of most of
> the column names but if you and others feel that is helpful then it is better to do
> it as part of Sawada-San's patch.
Yes. On this point, it was a mistake that I handled that changes.
It'd be better that Sawada-san takes care of them once the names have been fixed.


Best Regards,
    Takamichi Osumi