Re: PANIC serves too many masters

Hi,

Thanks for the review!

On Fri, 2 Oct 2026 at 13:36, Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
>
> On Fri, 18 Sept 2026 at 20:13, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> >
> > On Sat, 12 Sept 2026 at 21:50, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> > >
> > > I gave the core-dump part of this a try. Two WIP patches attached.
> > >
> > > I first thought of adding PANIC_NO_CORE, but wasn't sure where to put
> > > it. Below PANIC, we'd have to adjust checks like elevel >= PANIC.
> > > Above PANIC, it could take precedence over an ordinary PANIC during
> > > nested error reporting. Neither seemed quite right when all I wanted
> > > was to avoid the core dump.
>
> I found only one place which does 'elevel >= PANIC', in elog.c:
>
> if (elevel >= PANIC)
> {
> /*
> * Serious crash time. Postmaster will observe SIGABRT process exit
> * status and kill the other backends too.
> *
> * XXX: what if we are *in* the postmaster? abort() won't kill our
> * children...
> */
> fflush(NULL);
> abort();
> }
>
> I think this place is easy to change, but determining when to core
> dump (i.e. setting the correct error level) might be hard without
> looking at the error code itself. We can set the error level by
> looking at the error code, that might be another approach.

I had errstart() in mind too, where it promotes errors in critical
sections and takes the Max() over the stack. So v3 keeps the levels as
they are and only controls whether we call abort().

> > >
> > > Would something along these lines make sense?
> >
> > Attached is v2, which sets io_method=worker for the TAP test.
>
> I have reviewed v2-0001 of this patch and have a couple of comments.
>
> 1. What do you think about generalizing errnocoredump_on_errno()
> function with errabort($bool) like what Jeff suggested [1] upthread?
> Using a more general function will work better than passing errno to a
> specific function, IMO.

Added errabort(bool), but kept the errno helper for the WAL callers.
AFAICS dropping it would mean saving errno at each call site, since
ereport() doesn't allow it in the argument list. Do you think the
wrapper is worth keeping, or should the callers do that themselves?

> 2. The test coverage looks useful but I think it is too complicated
> for this feature. Could we simplify the tests? Also, I am not sure we
> need tests for this at all, but I don't have a strong opinion.

I pulled out the repeated crash/restart checks and removed the unused
branches in the callbacks.
I think we should keep the promoted ERROR, copied ErrorData and nested
PANIC cases?

> 3.
>
> @@ -611,13 +624,16 @@ errfinish(const char *filename, int lineno,
> const char *funcname)
> if (elevel >= PANIC)
> {
> /*
> - * Serious crash time. Postmaster will observe SIGABRT process exit
> - * status and kill the other backends too.
> + * Serious crash time. If this error is marked not to dump core, exit
> + * without running cleanup callbacks. Exit code 2 makes the postmaster
> + * treat this as a crash and kill the other backends too.
> *
> * XXX: what if we are *in* the postmaster? abort() won't kill our
> * children...
> */
>
> Could this comment mention both termination paths? Exit code 2 and
> termination by SIGABRT both trigger the postmaster's crash handling.
> The revised wording explains the former but omits the explanation of
> the latter.

Thanks, done.

> 4.
>
> @@ -1652,6 +1668,22 @@ errhidecontext(bool hide_ctx)
> return 0; /* return value does not matter */
> }
>
> +/*
> + * errnocoredump_on_errno --- skip a core dump for the specified errno
> + */
> +int
> +errnocoredump_on_errno(int errnum)
> +{
> + ErrorData *edata = &errordata[errordata_stack_depth];
> +
> + /* we don't bother incrementing recursion_depth */
> + CHECK_STACK_DEPTH();
> +
> + edata->no_core_dump |= edata->saved_errno == errnum;
>
> Once no_core_dump is set to true, we can't go back to false; which
> means we can't force core dumps for specific errors. I am not sure
> this is a problem now, but I think this is a limitation worth
> considering.

The |= in errnocoredump_on_errno() was so that a later non-matching
call wouldn't undo an earlier match. In v3 errabort() sets
no_core_dump directly, so errabort(true) can undo an earlier
errabort(false), and there's a test for that. The check in errfinish()
is unchanged though, since a nested error shouldn't suppress the core
we'd have taken for the original PANIC. Thoughts?

PFA v3, with latest updates.

Regards,
Ayush

> [1]
> ereport(PANIC,
> (errmsg("could not locate a valid checkpoint record"),
> errabort(false),errrestart(false)));