Re: Failed transaction statistics to measure the logical replication progress

On Sat, Nov 20, 2021 at 3:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 19, 2021 at 7:41 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Nov 18, 2021 at 12:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Nov 17, 2021 at 7:12 PM osumi.takamichi@fujitsu.com
> > > <osumi.takamichi@fujitsu.com> wrote:
> > > >
> > > > On Wednesday, November 17, 2021 10:00 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > 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.
> > > >
> > >
> > > Okay, I see your point and it makes sense to rename columns after
> > > these other stats. I am not able to come up with any better names than
> > > what is being used here. Sawada-San, do you agree with this, or do let
> > > us know if you have any better ideas?
> > >
> >
> > I'm concerned that these new names will introduce confusion; if we
> > have last_error_relid, last_error_command, last_error_message,
> > last_error_time, and last_error_xid, I think users might think that
> > first_error_time is the timestamp at which an error occurred for the
> > first time in the subscription worker.
> >
>
> Isn't to some extent that confusion already exists because of
> last_error_time column?
>
> > Also, I'm not sure
> > last_error_count is not clear to me (it looks like showing something
> > count but the only "last" one?).
> >
>
> I feel if all the error related columns have "last_error_" as a prefix
> then it should not be that confusing?
>
> > An alternative idea would be to add
> > total_error_count by this patch, resulting in having both error_count
> > and total_error_count. Regarding commit_count and abort_count, I
> > personally think xact_commit and xact_rollback would be better since
> > they’re more consistent with pg_stat_database view, although we might
> > already have discussed that.
> >
>
> Even if we decide to change the column names to
> xact_commit/xact_rollback, I think with additional non-error columns
> it will be clear to add 'error' in column names corresponding to error
> columns, and last_error_* seems to be consistent with what we have in
> pg_stat_archiver (last_failed_wal, last_failed_time).

Okay, I agree that last_error_* columns will be consistent.

> Your point
> related to first_error_time has merit and I don't have a better answer
> for it. I think it is just a convenience column and we are not sure
> whether that will be required in practice so maybe we can drop that
> column and come back to it later once we get some field feedback on
> this view?

+1

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/