Hi, On Mon, 12 Oct 2026 at 17:34, Nazir Bilal Yavuz <[email protected]> wrote: > > Hi, > > On Mon, 12 Oct 2026 at 10:04, Ayush Tiwari <[email protected]> > wrote: > > > > On Fri, 2 Oct 2026 at 16:38, Ayush Tiwari <[email protected]> > > wrote: > > > > > > On Fri, 2 Oct 2026 at 13:36, Nazir Bilal Yavuz <[email protected]> wrote: > > > > > > > > On Fri, 18 Sept 2026 at 20:13, Ayush Tiwari > > > > <[email protected]> wrote: > > > > > > > > > > On Sat, 12 Sept 2026 at 21:50, Ayush Tiwari > > > > > <[email protected]> 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.
Thanks for the review!
> > > > 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?
Ahh yes, added, thanks.
> > > > 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.
>
> 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 */
My bad, added them in v5.
>
> 0002: Changes look correct but perhaps you can add
> errnocoredump_on_errno(ENOSPC) to XLogWalRcvWrite() in walreceiver.c.
Yes, that write-error PANIC should get the same treatment. Added it.
> 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.
Done.
> 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'?
>
Hmm, that sounds better, I've renamed it as you suggested.
v5 attached with all the above changes.
Regards,
Ayush
v5-0001-Allow-selected-PANIC-errors-to-skip-core-dumps.patch
Description: Binary data
v5-0003-Test-PANIC-core-dump-suppression.patch
Description: Binary data
v5-0002-Avoid-core-dumps-for-WAL-disk-full-failures.patch
Description: Binary data
