Re: Accommodate startup process in a separate ProcState array slot instead of in MaxBackends slots. - Mailing list pgsql-hackers

В Сб, 12/02/2022 в 16:56 +0530, Bharath Rupireddy пишет:
> On Fri, Feb 11, 2022 at 7:56 PM Yura Sokolov <[email protected]> wrote:
> > В Сб, 16/10/2021 в 16:37 +0530, Bharath Rupireddy пишет:
> > > On Thu, Oct 14, 2021 at 10:56 AM Fujii Masao
> > > <[email protected]> wrote:
> > > > On 2021/10/12 15:46, Bharath Rupireddy wrote:
> > > > > On Tue, Oct 12, 2021 at 5:37 AM Fujii Masao <[email protected]> wrote:
> > > > > > On 2021/10/12 4:07, Bharath Rupireddy wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > While working on [1], it is found that currently the ProcState array
> > > > > > > doesn't have entries for auxiliary processes, it does have entries for
> > > > > > > MaxBackends. But the startup process is eating up one slot from
> > > > > > > MaxBackends. We need to increase the size of the ProcState array by 1
> > > > > > > at least for the startup process. The startup process uses ProcState
> > > > > > > slot via InitRecoveryTransactionEnvironment->SharedInvalBackendInit.
> > > > > > > The procState array size is initialized to MaxBackends in
> > > > > > > SInvalShmemSize.
> > > > > > > 
> > > > > > > The consequence of not fixing this issue is that the database may hit
> > > > > > > the error "sorry, too many clients already" soon in
> > > > > > > SharedInvalBackendInit.
> > > > 
> > > > On second thought, I wonder if this error could not happen in practice. No?
> > > > Because autovacuum doesn't work during recovery and the startup process
> > > > can safely use the ProcState entry for autovacuum worker process.
> > > > Also since the minimal allowed value of autovacuum_max_workers is one,
> > > > the ProcState array guarantees to have at least one entry for autovacuum worker.
> > > > 
> > > > If this understanding is right, we don't need to enlarge the array and
> > > > can just update the comment. I don't strongly oppose to enlarge
> > > > the array in the master, but I'm not sure it's worth doing that
> > > > in back branches if the issue can cause no actual error.
> > > 
> > > Yes, the issue can't happen. The comment in the SInvalShmemSize,
> > > mentioning about the startup process always having an extra slot
> > > because the autovacuum worker is not active during recovery, looks
> > > okay. But, is it safe to assume that always? Do we have a way to
> > > specify that in the form an Assert(when_i_am_startup_proc &&
> > > autovacuum_not_running) (this looks a bit dirty though)? Instead, we
> > > can just enlarge the array in the master and be confident about the
> > > fact that the startup process always has one dedicated slot.
> > 
> > But this slot wont be used for most of cluster life. It will be just
> > waste.
> 
> Correct. In the standby autovacuum launcher and worker are not started
> so, the startup process will always have a slot free for it to use.
> 
> > And `Assert(there_is_startup_proc && autovacuum_not_running)` has
> > value on its own, hasn't it? So why doesn't add it with comment.
> 
> Assertion doesn't make sense to me now. Because the postmaster ensures
> that the autovacuum launcher/workers will not get started in standby
> mode and we can't reliably know in InitRecoveryTransactionEnvironment
> (startup process) whether or not autovacuum launcher process has been
> started.
> 
> FWIW, here's a patch just adding a comment on how the startup process
> can get a free procState array slot even when SInvalShmemSize hasn't
> accounted for it.

I think, comment is a good thing.
Marked as "Ready for committer".

> 
> Regards,
> Bharath Rupireddy.