Re: PANIC serves too many masters

Hi,

On Mon, 12 Oct 2026 at 10:04, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
>
> On Fri, 2 Oct 2026 at 16:38, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> >
> > 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:
> > > > >
> > > > > 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?

I think keeping it as a wrapper makes sense. It uses saved_errno and
avoids having each WAL caller save errno before ereport(). That seems
nice to me.

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

That looks good. One small coverage point: what aboud adding a case
which suppresses the core dump and then calls errabort(true), with the
force_core_dump_on_panic GUC off?

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

LGTM.

> For ease of review, I've put the tests in 0003, leaving 0001 and 0002
> focused on the core and WAL changes. The core and WAL code is unchanged
> from v3.

Thanks, this version is easier to review.

Code comments:

0001:

1.

+/*
+ * errabort --- set whether a PANIC should call abort()
+ */
+int
+errabort(bool do_abort)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ CHECK_STACK_DEPTH();
+ edata->no_core_dump = !do_abort;
+
+ return 0;
+}

It is cosmetic but it seems you forgot these comments:

/* we don't bother incrementing recursion_depth */
and
/* return value does not matter */

0002: Changes look correct but perhaps you can add
errnocoredump_on_errno(ENOSPC) to XLogWalRcvWrite() in walreceiver.c.

0003:

1.

+static void
+#ifdef WIN32
+injection_abort_handler(int signal)
+#else
+injection_abort_handler(SIGNAL_ARGS)
+#endif
+{
+ _exit(42);
+}
+

I think this function needs a comment. It is hard to understand this
function's purpose and also the special number.

2.

+/* Make abort() observable without producing a core file. */
+PG_FUNCTION_INFO_V1(injection_points_panic);

This comment doesn't seem relevant to the EIO input. Something like
this 'Test PANIC core-dump suppression with ENOSPC or EIO.' can fit
better. Also, function name is a bit general, what about something
like 'injection_points_test_panic_core_dump'?

--
Regards,
Nazir Bilal Yavuz
Microsoft