Date: Wed, 14 Dec 2022 09:10:23 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Melih Mutlu <m.melihmutlu@gmail.com>,
	Thomas Munro <thomas.munro@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: wake up logical workers after ALTER SUBSCRIPTION
Message-ID: <20221214171023.GA689106@nathanxps13>
References: <20221202192101.GB2277157@nathanxps13>
 <CAGPVpCTUuKyxB5f3exgDMBRTivbN5uHVkD=YfXmgznrdnwVOQw@mail.gmail.com>
 <20221206192551.GA3078082@nathanxps13>
 <20221206212954.GA3403597@nathanxps13>
 <CAGPVpCTkdaOyBLXB435VXq0RA1cHLFQut7BPm6t8D4dsdiEXZQ@mail.gmail.com>
 <20221207181145.GA3698731@nathanxps13>
 <2550453.1670974328@sss.pgh.pa.us>
 <20221214000145.GA638663@nathanxps13>
 <2561155.1670977214@sss.pgh.pa.us>
 <20221214004105.GA669835@nathanxps13>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221214004105.GA669835@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20221214171023.GA689106%40nathanxps13>
Precedence: bulk

On Tue, Dec 13, 2022 at 04:41:05PM -0800, Nathan Bossart wrote:
> On Tue, Dec 13, 2022 at 07:20:14PM -0500, Tom Lane wrote:
>> I certainly don't think that "wake the apply launcher every 1ms"
>> is a sane configuration.  Unless I'm missing something basic about
>> its responsibilities, it should seldom need to wake at all in
>> normal operation.
> 
> This parameter appears to control how often the apply launcher starts new
> workers.  If it starts new workers in a loop iteration, it updates its
> last_start_time variable, and it won't start any more workers until another
> wal_retrieve_retry_interval has elapsed.  If no new workers need to be
> started, it only wakes up every 3 minutes.

Looking closer, I see that wal_retrieve_retry_interval is used for three
purposes.  It's main purpose seems to be preventing busy-waiting in
WaitForWALToBecomeAvailable(), as that's what's documented.  But it's also
used for logical replication.  The apply launcher uses it as I've describe
above, and the apply workers use it when launching sync workers.  Unlike
the apply launcher, the apply workers store the last start time for each
table's sync worker and use that to determine whether to start a new one.

My first thought is that the latter two uses should be moved to a new
parameter, and the apply launcher should store the last start time for each
apply worker like the apply workers do for the table-sync workers.  In any
case, it probably makes sense to lower this parameter's value for testing
so that tests that restart these workers frequently aren't waiting for so
long.

I can put a patch together if this seems like a reasonable direction to go.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com