Re: Checkpoint start logging is done inside critical section

On Thu, Oct 18, 2018 at 10:27 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2018-10-18 10:21:39 +0530, Amit Kapila wrote:
> > On Thu, Oct 18, 2018 at 4:44 AM Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:
> > >
> > > The LogCheckpointStart() call inside CreateCheckPoint() is done while
> > > inside a critical section. The elog call could trigger errors due to
> > > memory allocations or from a logging hook, resulting in a panic.
> > >
> >
> > Yeah, but we use logging hook inside LWLockAcquire as well which is
> > also called inside critical section in the nearby code, not sure if we
> > can do anything about it.
>
> > > It
> > > seems better to postpone the logging until after the critical section
> > > is done. It's only a few lwlock acquisitions away and shouldn't make
> > > any material difference. Patch to do so is attached.
> > >
> >
> > +1. I think we should backpatch this as well. I can take it forward
> > unless people have objections to it.
>
> We do a fair bit of allocations inside a critical section during a
> checkpoint
>

The other possibility of allocation is during
LocalSetXLogInsertAllowed(), but we make provision for that before
starting critical section. Which other cases do you see where
allocation is possible? I think in general, we take care that we
don't allocate inside a critical section, otherwise, it will lead to
at the very least assertion failure unless we have marked the memory
context as "allowed in crit section".

> (that's why we mark the ctx as being ok with that).
>

Yeah, as the palloc for log message would be called in an ErrorContext
where it is safe to do the allocation, so ideally this shouldn't be a
problem. So, it seems to me that this is not a problem, Ants, do you
see any problem in any particular scenario or was this based on
theoretical analysis?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com