Feedback-ID: id4a34324:Fastmail
Date: Tue, 7 Apr 2026 22:20:55 -0400
From: Andres Freund <andres@anarazel.de>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Automatically sizing the IO worker pool
Message-ID: <v4e2phxr3n22lv6skhtczbhy3xbyqwxuy7pj7cdh6afmhwhrqb@k6cpnb2kjw6c>
References: 
 <CA+hUKG+P80oR7yy5-67uHqwBWr9rux69BkQF9fwz=f0LuDn3Rw@mail.gmail.com>
 <CA+hUKGKgVhLVS4Pne8cTR0f=tE8GtF4-CvY2HMDOJEm7w0iXfA@mail.gmail.com>
 <20260328095204.5tsq5bldugeumrtf@erthalion>
 <CA+hUKGLUZ51zkxUoBvANcLdvz8DjS_85=6985HE8gLuNtBxX7Q@mail.gmail.com>
 <jp7lhn2wng5mlhn3jvn4t6nmr23l6bnim5af3qslnoguevfqqz@d7cvksm62j4j>
 <CA+hUKGLp_yPztSoQkmRe1kFk1M5JNr7b+07cc+46AVg=VoKC-Q@mail.gmail.com>
 <adaz4sd2x64uoei2nmd4pinvothusd7uwllnilq6msfp4wytrw@npedbcvg3llt>
 <CA+hUKGJi6L2ftuTiKw2W4AAah6Uivs4JGiq58A7MjePbXpc4BA@mail.gmail.com>
 <4vhrgxmx5w4cjr7vgegur3hbkuojt2iz23v4dqfypsgvl5bszi@xrxefkkcxdja>
 <CA+hUKGK=vELXFXNj2L=vTkof6s_EQzTjYXXrUVwOOW0rahEfVg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+hUKGK=vELXFXNj2L=vTkof6s_EQzTjYXXrUVwOOW0rahEfVg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/v4e2phxr3n22lv6skhtczbhy3xbyqwxuy7pj7cdh6afmhwhrqb%40k6cpnb2kjw6c>
Precedence: bulk

Hi,

On 2026-04-08 14:09:16 +1200, Thomas Munro wrote:
> On Wed, Apr 8, 2026 at 12:30 PM Andres Freund <andres@anarazel.de> wrote:
> > On 2026-04-08 11:18:51 +1200, Thomas Munro wrote:
> > > >                 /* Choose one worker to wake for this batch. */
> > > >                 if (worker == -1)
> > > >                         worker = pgaio_worker_choose_idle(-1);
> > >
> > > Well I didn't want to wake a worker if we'd failed to enqueue
> > > anything.
> >
> > I think it's worth waking up workers if there are idle ones and the queue is
> > full?
> 
> True, but I prefer to test nsync because there is another reason to break:

I don't follow.  What I was proposing is after the conditional lock
acquisition succeeded.  So is your nsync == 0 check.

> +/*
> + * Tell postmaster that we think a new worker is needed.
> + */
> +static void
> +pgaio_worker_request_grow(void)
> +{
> +	/*
> +	 * Suppress useless signaling if we already know that we're at the
> +	 * maximum.  This uses an unlocked read of nworkers, but that's OK for
> +	 * this heuristic purpose.
> +	 */
> +	if (io_worker_control->nworkers < io_max_workers)
>  	{
> -		io_worker_control->workers[i].latch = NULL;
> -		io_worker_control->workers[i].in_use = false;
> +		if (!io_worker_control->grow)
> +		{
> +			io_worker_control->grow = true;
> +			pg_memory_barrier();
> +
> +			/*
> +			 * If the postmaster has already been signaled, don't do it again
> +			 * until the postmaster clears this flag.  There is no point in
> +			 * repeated signals if grow is being set and cleared repeatedly
> +			 * while the postmaster is waiting for io_worker_launch_interval
> +			 * (which it applies even to canceled requests).
> +			 */
> +			if (!io_worker_control->grow_signal_sent)
> +			{
> +				io_worker_control->grow_signal_sent = true;
> +				pg_memory_barrier();
> +				SendPostmasterSignal(PMSIGNAL_IO_WORKER_GROW);
> +			}
> +		}
>  	}
>  }


I'd probbly use early returns to make it a bit more readable.


> +static bool
> +pgaio_worker_can_timeout(void)
> +{
> +	PgAioWorkerSet workerset;
> +
> +	/* Serialize against pool size changes. */
> +	LWLockAcquire(AioWorkerControlLock, LW_SHARED);
> +	workerset = io_worker_control->workerset;
> +	LWLockRelease(AioWorkerControlLock);
> +
> +	if (MyIoWorkerId != pgaio_workerset_get_highest(&workerset))
> +		return false;
> +
> +	if (MyIoWorkerId < io_min_workers)
> +		return false;
> +
> +	return true;
> +}

I guess I'd move the < io_min_workers to earlier so that you don't acquire the
lock if that'll return false anyway.


Greetings,

Andres Freund