Subject: Re: JIT causes core dump during error recovery

From: Tom Lane <[email-masked]>
Date: Wed, 26 Jun 2024 16:01:47 -0400

I wrote:
> What I think is the right solution is to fix things so that
> seemingly-no-longer-used jit compilations are not thrown away
> until transaction cleanup.  I don't know the JIT code nearly
> well enough to take point on fixing it like that, though.

Or maybe not.  I found by bisecting that it doesn't fail before
2e517818f (Fix SPI's handling of errors during transaction commit).
A salient part of that commit message:

    Having made that API redefinition, we can fix this mess by having
    SPI_commit[_and_chain] trap errors and start a new, clean transaction
    before re-throwing the error.  Likewise for SPI_rollback[_and_chain].

So delaying removal of the jit-created code segment until transaction
cleanup wouldn't be enough to prevent this crash, if I'm reading
things right.  The extra-pstrdup solution may be the only viable one.

I could use confirmation from someone who knows the JIT code about
when jit-created code is unloaded.  It also remains very unclear
why there is no crash if we don't force both jit_optimize_above_cost
and jit_inline_above_cost to small values.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Thu, 27 Jun 2024 12:18:31 -0400

> and jit_inline_above_cost to small values.

I found where the unload happens: ResOwnerReleaseJitContext, which
is executed during the resource owner BEFORE_LOCKS phase.  (Which
seems like a pretty dubious choice from here; why can't we leave
it till the less time-critical phase after we've let go of locks?)
But anyway, we definitely do drop this stuff during xact cleanup.

Also, it seems that the reason that both jit_optimize_above_cost
and jit_inline_above_cost must be small is that otherwise int4div
is simply called from the JIT-generated code, not inlined into it.
This gives me very considerable fear about how well that behavior
has been tested: if throwing an error from inlined code doesn't
work, and we hadn't noticed that, how much can it really have been
exercised?  I have also got an itchy feeling that we have code that
will be broken by this behavior of "if it happens to get inlined
then string constants aren't so constant".

In any case, I found that adding some copying logic to CopyErrorData()
is enough to solve this problem, since the SPI infrastructure applies
that before executing xact cleanup.  I had feared that we'd have to
add copying to every single elog/ereport sequence, which would have
been an annoying amount of overhead; but the attached seems
acceptable.  We do get through check-world with this patch and the
JIT parameters all set to small values.

			regards, tom lane

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index d91a85cb2d..d1d1632bdd 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1743,7 +1743,21 @@ CopyErrorData(void)
 	newedata = (ErrorData *) palloc(sizeof(ErrorData));
 	memcpy(newedata, edata, sizeof(ErrorData));
 
-	/* Make copies of separately-allocated fields */
+	/*
+	 * Make copies of separately-allocated strings.  Note that we copy even
+	 * theoretically-constant strings such as filename.  This is because those
+	 * could point into JIT-created code segments that might get unloaded at
+	 * transaction cleanup.  In some cases we need the copied ErrorData to
+	 * survive transaction boundaries, so we'd better copy those strings too.
+	 */
+	if (newedata->filename)
+		newedata->filename = pstrdup(newedata->filename);
+	if (newedata->funcname)
+		newedata->funcname = pstrdup(newedata->funcname);
+	if (newedata->domain)
+		newedata->domain = pstrdup(newedata->domain);
+	if (newedata->context_domain)
+		newedata->context_domain = pstrdup(newedata->context_domain);
 	if (newedata->message)
 		newedata->message = pstrdup(newedata->message);
 	if (newedata->detail)
@@ -1756,6 +1770,8 @@ CopyErrorData(void)
 		newedata->context = pstrdup(newedata->context);
 	if (newedata->backtrace)
 		newedata->backtrace = pstrdup(newedata->backtrace);
+	if (newedata->message_id)
+		newedata->message_id = pstrdup(newedata->message_id);
 	if (newedata->schema_name)
 		newedata->schema_name = pstrdup(newedata->schema_name);
 	if (newedata->table_name)

---

From: Ranier Vilela <[email-masked]>
Date: Thu, 27 Jun 2024 13:31:11 -0300

Em qui., 27 de jun. de 2024 às 13:18, Tom Lane <[email-masked]> escreveu:

> that before executing xact cleanup.

In this case, I think that these fields, in struct definition struct
ErrorData (src/include/utils/elog.h)
should be changed too?
from const char * to char*

best regards,
Ranier Vilela

---

From: Tom Lane <[email-masked]>
Date: Thu, 27 Jun 2024 12:59:37 -0400

> from const char * to char*

No, that would imply casting away const in errstart() etc.  We're
still mostly expecting those things to be pointers to constant
strings.

I'm about half tempted to file this as an LLVM bug.  When it inlines
a function, it should still reference the same string constants that
the original code did, otherwise it's failing to be a transparent
conversion.  But they'll probably cite some standards-ese that claims
this is undefined behavior:

	const char * foo(void) { return "foo"; }

	void bar(void) { Assert( foo() == foo() ); }

on which I call BS, but it's probably in there somewhere.

			regards, tom lane