On 12/04/17 05:41, Peter Eisentraut wrote:
> On 4/10/17 13:06, Stas Kelvich wrote:
>>
>>> On 10 Apr 2017, at 19:50, Peter Eisentraut 
>>> <peter.eisentr...@2ndquadrant.com> wrote:
>>>
>>> On 4/10/17 05:49, Stas Kelvich wrote:
>>>> Here is small patch to call statistics in logical worker. Originally i 
>>>> thought that stat
>>>> collection during logical replication should manually account amounts of 
>>>> changed tuples,
>>>> but seems that it is already smoothly handled on relation level. So call 
>>>> to 
>>>> pgstat_report_stat() is enough.
>>>
>>> I wonder whether we need a similar call somewhere in tablesync.c.  It
>>> seems to work without it, though.
>>
>> I thought it spins up the same worker from worker.c.
> 
> It depends on which of the various tablesync scenarios happens.  If the
> apply lags behind the tablesync, then the apply doesn't process commit
> messages until it has caught up.  So the part of the code you patched
> wouldn't be called.
> 

Indeed, if catchup phase didn't happen (because tablesync was faster
than apply) then the commit handler is never called so all the changes
made by copy would be forgotten. Also the tablesync worker might exit
from process_syncing_tables() call which is called before we report
stats in the commit handler so again some changes might be forgotten.

I attached modified version of the patch that also reports stats in
finish_sync_worker() when there is outstanding transaction. The test can
stay the same.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 231973cafe12bb948924a6380ac785d93b6af086 Mon Sep 17 00:00:00 2001
From: Petr Jelinek <pjmo...@pjmodos.net>
Date: Fri, 14 Apr 2017 17:54:03 +0200
Subject: [PATCH] Report statistics in logical replication workers

---
 src/backend/postmaster/pgstat.c             | 9 +++++----
 src/backend/replication/logical/tablesync.c | 8 +++++++-
 src/backend/replication/logical/worker.c    | 1 +
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 3fb57f0..36fedd8 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -769,10 +769,11 @@ allow_immediate_pgstat_restart(void)
 /* ----------
  * pgstat_report_stat() -
  *
- *	Called from tcop/postgres.c to send the so far collected per-table
- *	and function usage statistics to the collector.  Note that this is
- *	called only when not within a transaction, so it is fair to use
- *	transaction stop time as an approximation of current time.
+ *	Must be called by processes that performs DML: tcop/postgres.c, logical
+ *	receiver processes, SPI worker, etc to send the so far collected per-table
+ *	and function usage statistics to the collector. Note that this is called
+ *	only when not within a transaction, so it is fair to use transaction stop
+ *	time as an approximation of current time.
  * ----------
  */
 void
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index d1f2734..aa1a85b 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -114,9 +114,15 @@ StringInfo	copybuf = NULL;
 static void pg_attribute_noreturn()
 finish_sync_worker(void)
 {
-	/* Commit any outstanding transaction. */
+	/*
+	 * Commit any outstanding transaction. This is the usual case, unless
+	 * there was nothing to do for the table.
+	 */
 	if (IsTransactionState())
+	{
 		CommitTransactionCommand();
+		pgstat_report_stat(false);
+	}
 
 	/* And flush all writes. */
 	XLogFlush(GetXLogWriteRecPtr());
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 3313448..169471c 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -462,6 +462,7 @@ apply_handle_commit(StringInfo s)
 	/* Process any tables that are being synchronized in parallel. */
 	process_syncing_tables(commit_data.end_lsn);
 
+	pgstat_report_stat(false);
 	pgstat_report_activity(STATE_IDLE, NULL);
 }
 
-- 
2.7.4

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to