Re: PANIC serves too many masters - Mailing list pgsql-hackers

Hi,

On Fri, 18 Sept 2026 at 20:13, Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:
>
> On Sat, 12 Sept 2026 at 21:50, Ayush Tiwari <ayushtiwari.slg01@gmail.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.


> >
> > 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.


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.


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.


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.


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

-- 
Regards,
Nazir Bilal Yavuz
Microsoft