public inbox for [email protected]  
help / color / mirror / Atom feed
From: [email protected] <[email protected]>
To: [email protected] <[email protected]>
To: 'Greg Nancarrow' <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: RE: Failed transaction statistics to measure the logical replication progress
Date: Fri, 31 Dec 2021 01:12:25 +0000
Message-ID: <TYCPR01MB6128932B793A2E0517421607FB469@TYCPR01MB6128.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <TYCPR01MB8373176545F7AE2CCDA23B81ED7D9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
References: <CALDaNm0Wg8XwfqUVnBSSahOtNxGDLNOW1_tc4ikZK=tGfqje-A@mail.gmail.com>
	<TYCPR01MB8373A9B6508E44E547A98EF5ED779@TYCPR01MB8373.jpnprd01.prod.outlook.com>
	<TYCPR01MB83734A7A0596AC7ADB0DCB51ED779@TYCPR01MB8373.jpnprd01.prod.outlook.com>
	<[email protected]>
	<TYCPR01MB83736371B66F90C71365ED11ED7B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
	<CAJcOf-dmfinJDr=nOmjX1qNiU_mvW5tG3GyVwsQzRnUBb2ky0A@mail.gmail.com>
	<TYCPR01MB8373176545F7AE2CCDA23B81ED7D9@TYCPR01MB8373.jpnprd01.prod.outlook.com>

On Wednesday, December 22, 2021 6:14 PM [email protected] <[email protected]> wrote:
> 
> Attached the new patch v19.
> 

Thanks for your patch. I think it's better if you could add this patch to the commitfest.
Here are some comments:

1)
+       <structfield>commit_count</structfield> <type>bigint</type>
+      </para>
+      <para>
+       Number of transactions successfully applied in this subscription.
+       Both COMMIT and COMMIT PREPARED increment this counter.
+      </para></entry>
+     </row>
...

I think the commands (like COMMIT, COMMIT PREPARED ...) can be surrounded with
"<command> </command>", thoughts?

2)
+extern void pgstat_report_subworker_xact_end(LogicalRepWorker *repWorker,
+											 LogicalRepMsgType command,
+											 bool bforce);

Should "bforce" be "force"?

3)
+ *  This should be called before the call of process_syning_tables() so to not

"process_syning_tables()" should be "process_syncing_tables()".

4)
+void
+pgstat_send_subworker_xact_stats(LogicalRepWorker *repWorker, bool force)
+{
+	static TimestampTz last_report = 0;
+	PgStat_MsgSubWorkerXactEnd msg;
+
+	if (!force)
+	{
...
+		if (!TimestampDifferenceExceeds(last_report, now, PGSTAT_STAT_INTERVAL))
+			return;
+		last_report = now;
+	}
+
...
+	if (repWorker->commit_count == 0 && repWorker->abort_count == 0)
+		return;
...

I think it's better to check commit_count and abort_count first, then check if
reach PGSTAT_STAT_INTERVAL.
Otherwise if commit_count and abort_count are 0, it is possible that the value
of last_report has been updated but it didn't send stats in fact. In this case,
last_report is not the real time that send last message.

Regards,
Tang


view thread (113+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: RE: Failed transaction statistics to measure the logical replication progress
  In-Reply-To: <TYCPR01MB6128932B793A2E0517421607FB469@TYCPR01MB6128.jpnprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox