RE: Failed transaction statistics to measure the logical replication progress

On Friday, February 18, 2022 3:34 PM Tang, Haiying/唐 海英 <tanghy(dot)fnst(at)fujitsu(dot)com> wrote:
> On Wed, Jan 12, 2022 8:35 PM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> >
> > The attached v21 has a couple of other minor updates like a
> > modification of error message text.
> >
> >
>
> Thanks for updating the patch. Here are some comments.
Thank you for your reivew !

> 1) I saw the following description about pg_stat_subscription_workers view in
> existing doc:
>
> The <structname>pg_stat_subscription_workers</structname> view will
> contain
> one row per subscription worker on which errors have occurred, ...
>
> It only says "which errors have occurred", maybe we should also mention
> transactions here, right?
I wrote about this statistics in the next line but as you pointed out,
separating the description into two sentences wasn't good idea.
Fixed.

> 2)
> /* ----------
> + * pgstat_send_subworker_xact_stats() -
> + *
> + * Send a subworker's transaction stats to the collector.
> + * The statistics are cleared upon return.
>
> Should "The statistics are cleared upon return" changed to "The statistics are
> cleared upon sending"? Because if it doesn't reach PGSTAT_STAT_INTERVAL
> and the transaction stats are not sent, the function will return without clearing
> out statistics.
Now, the purpose of this function has become purely
to send a message and whenever it's called, the function
clears the saved stats. So, I skipped this comments now.

> 3)
> + Assert(command == LOGICAL_REP_MSG_COMMIT ||
> + command == LOGICAL_REP_MSG_STREAM_COMMIT ||
> + command == LOGICAL_REP_MSG_COMMIT_PREPARED
> ||
> + command ==
> LOGICAL_REP_MSG_ROLLBACK_PREPARED);
> +
> + switch (command)
> + {
> + case LOGICAL_REP_MSG_COMMIT:
> + case LOGICAL_REP_MSG_STREAM_COMMIT:
> + case LOGICAL_REP_MSG_COMMIT_PREPARED:
> + MyLogicalRepWorker->commit_count++;
> + break;
> + case LOGICAL_REP_MSG_ROLLBACK_PREPARED:
> + MyLogicalRepWorker->abort_count++;
> + break;
> + default:
> + ereport(ERROR,
> + errmsg("invalid logical message type
> for transaction statistics of subscription"));
> + break;
> + }
>
> I'm not sure that do we need the assert, because it will report an error later if
> command is an invalid value.
The Assert has been removed, along with the switch branches now.
Since there was an adivce that we should call this from apply_handle_commit_internal
and from that function, if we don't want to change this function's argument,
all we need to do is to pass a boolean value that indicates the stats is
commit_count or abort_count. Kindly have a look at the updated version.

> 4) I noticed that the abort_count doesn't include aborted streaming
> transactions.
> Should we take this case into consideration?
Hmm, we can add this into this column, when there's no objection.
I'm not sure but someone might say those should be separate columns.

The new patch v22 is shared in [2].

[2] - https://www.postgresql.org/message-id/TYCPR01MB83737C689F8F310C87C19C1EED379%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
Takamichi Osumi