RE: Failed transaction statistics to measure the logical replication progress

On Thursday, November 18, 2021 8:35 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> On Tue, Nov 16, 2021 at 6:04 PM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)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.
>
> Thanks for the updated patch, Few comments:
Thank you for checking the patches !

> 1) since pgstat_get_subworker_prepared_txn is called from only one place and
> create is passed as true, we can remove create function parameter or the
> function could be removed.
> + * Return subscription worker entry with the given subscription OID and
> + * gid.
> + * ----------
> + */
> +static PgStat_SW_PreparedXactEntry *
> +pgstat_get_subworker_prepared_txn(Oid databaseid, Oid subid,
> + char
> *gid, bool create)
> +{
> + PgStat_StatDBEntry *dbentry;
> + PgStat_SW_PreparedXactKey key;
Removed the parameter.

> 2) Include subworker prepared transactions also
> /*
> * Don't create tables/functions/subworkers hashtables for
> * uninteresting databases.
> */
> if (onlydb != InvalidOid)
> {
> if (dbbuf.databaseid != onlydb &&
> dbbuf.databaseid != InvalidOid)
> break;
> }
Fixed.

> 3) Similarly it should be mentioned in:
> reset_dbentry_counters function header, pgstat_read_db_statsfile function
> header and pgstat_get_db_entry function comments.
Fixed.

> 4) I felt we can remove "COMMIT of streaming transaction", since only commit
> and commit prepared are the user operations. Shall we change it to "COMMIT
> and COMMIT PREPARED will increment this counter."
> + <structfield>commit_count</structfield> <type>bigint</type>
> + </para>
> + <para>
> + Number of transactions successfully applied in this subscription.
> + COMMIT, COMMIT of streaming transaction and COMMIT
> PREPARED increments
> + this counter.
> + </para></entry>
> + </row>
You are right ! Fixed.

> 5) PgStat_SW_PreparedXactEntry should be before
> PgStat_SW_PreparedXactKey PgStat_StatSubWorkerEntry
> PgStat_StatSubWorkerKey
> +PgStat_SW_PreparedXactKey
> +PgStat_SW_PreparedXactEntry
> PgStat_StatTabEntry
> PgStat_SubXactStatus
Fixed.

> 6) This change is not required
> @@ -293,6 +306,7 @@ static inline void cleanup_subxact_info(void); static
> void stream_cleanup_files(Oid subid, TransactionId xid); static void
> stream_open_file(Oid subid, TransactionId xid, bool first); static void
> stream_write_change(char action, StringInfo s);
> +
> static void stream_close_file(void);
Removed.

Other changes are
1. refined the commit message of v13-0003*.
2. made the additional comment for ApplyErrorCallbackArg simple.
3. wrote more explanations about update_apply_change_size() as comment.
4. changed the behavior of pgstat_recv_subworker_error so that
it can store stats info only once per error.
5. added one simple test for PREPARE and COMMIT PREPARED.

This used v23 skip xid patch [1].
(I will remove v13-0001* when the column names are fixed
and Sawada-san starts to take care of the column name definitions)

[1] - https://www.postgresql.org/message-id/CAD21AoA5jupM6O%3DpYsyfaxQ1aMX-en8%3DQNgpW6KfXsg7_CS0CQ%40mail.gmail.com

Best Regards,
Takamichi Osumi