Re: [HACKERS] A little improvementof ApplyLauncherMain loop code

[Yugo Nagata]

On Tue, 15 Aug 2017 15:17:06 -0400
Peter Eisentraut  wrote:

> On 8/1/17 02:28, Yugo Nagata wrote:
> > When reading the logical replication code, I found that the following
> > part could be improved a bit. In the foreach, LWLockAcquire and
> > logicalrep_worker_find are called for each loop, but they are needed
> > only when sub->enabled is true.
> 
> Fixed, thanks!

Thanks, too!

> 
> -- 
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Yugo Nagata 


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

Re: [HACKERS] A little improvementof ApplyLauncherMain loop code

[Peter Eisentraut]

On 8/1/17 02:28, Yugo Nagata wrote:
> When reading the logical replication code, I found that the following
> part could be improved a bit. In the foreach, LWLockAcquire and
> logicalrep_worker_find are called for each loop, but they are needed
> only when sub->enabled is true.

Fixed, thanks!

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

[HACKERS] A little improvementof ApplyLauncherMain loop code

[Yugo Nagata]

Hi,

When reading the logical replication code, I found that the following
part could be improved a bit. In the foreach, LWLockAcquire and
logicalrep_worker_find are called for each loop, but they are needed
only when sub->enabled is true.

 846 /* Start the missing workers for enabled subscriptions. */
 847 foreach(lc, sublist)
 848 {
 849 Subscription *sub = (Subscription *) lfirst(lc);
 850 LogicalRepWorker *w;
 851 
 852 LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
 853 w = logicalrep_worker_find(sub->oid, InvalidOid, false);
 854 LWLockRelease(LogicalRepWorkerLock);
 855 
 856 if (sub->enabled && w == NULL)
 857 {
 858 last_start_time = now;
 859 wait_time = wal_retrieve_retry_interval;
 860 
 861 logicalrep_worker_launch(sub->dbid, sub->oid, 
sub->name,
 862  sub->owner, InvalidOid);
 863 }
 864 }

We can fix this to call them only when there are needed, but I'm not sure
whether these call are expensive enough to fix. Is it worth to fix? 
A patch attached.

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index d165d51..4816a5b 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -849,11 +849,14 @@ ApplyLauncherMain(Datum main_arg)
 Subscription *sub = (Subscription *) lfirst(lc);
 LogicalRepWorker *w;
 
+if (!sub->enabled)
+	continue;
+
 LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
 w = logicalrep_worker_find(sub->oid, InvalidOid, false);
 LWLockRelease(LogicalRepWorkerLock);
 
-if (sub->enabled && w == NULL)
+if (w == NULL)
 {
 	last_start_time = now;
 	wait_time = wal_retrieve_retry_interval;

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