Re: SQL/JSON features for v15

Hi,

On 2022-08-15 15:38:53 -0700, Andres Freund wrote:
> Next question:
> 
> 	/*
> 	 * We should catch exceptions of category ERRCODE_DATA_EXCEPTION and
> 	 * execute the corresponding ON ERROR behavior then.
> 	 */
> 	oldcontext = CurrentMemoryContext;
> 	oldowner = CurrentResourceOwner;
> 
> 	Assert(error);
> 
> 	BeginInternalSubTransaction(NULL);
> 	/* Want to execute expressions inside function's memory context */
> 	MemoryContextSwitchTo(oldcontext);
> 
> 	PG_TRY();
> 	{
> 		res = func(op, econtext, res, resnull, p, error);
> 
> 		/* Commit the inner transaction, return to outer xact context */
> 		ReleaseCurrentSubTransaction();
> 		MemoryContextSwitchTo(oldcontext);
> 		CurrentResourceOwner = oldowner;
> 	}
> 	PG_CATCH();
> 	{
> 		ErrorData  *edata;
> 		int			ecategory;
> 
> 		/* Save error info in oldcontext */
> 		MemoryContextSwitchTo(oldcontext);
> 		edata = CopyErrorData();
> 		FlushErrorState();
> 
> 		/* Abort the inner transaction */
> 		RollbackAndReleaseCurrentSubTransaction();
> 		MemoryContextSwitchTo(oldcontext);
> 		CurrentResourceOwner = oldowner;
> 
> 
> Two points:
> 
> 1) I suspect it's not safe to switch to oldcontext before calling func().
> 
> On error we'll have leaked memory into oldcontext and we'll just continue
> on. It might not be very consequential here, because the calling context
> presumably isn't very long lived, but that's probably not something we should
> rely on.
> 
> Also, are we sure that the context will be in a clean state when it's used
> within an erroring subtransaction?
> 
> 
> I think the right thing here would be to stay in the subtransaction context
> and then copy the datum out to the surrounding context in the success case.
> 
> 
> 2) If there was an out-of-memory error, it'll have been in oldcontext. So
> switching back to it before calling CopyErrorData() doesn't seem good - we'll
> just hit OOM issues again.
> 
> 
> I realize that both of these issues are present in plenty other code (see
> e.g. plperl_spi_exec()). So I'm curious why they are ok?

Certainly seems to be missing a FreeErrorData() for the happy path?


It'd be nicer if we didn't copy the error. In the case we rethrow we don't
need it, because we can just PG_RE_THROW(). And in the other path we just want
to get the error code. It just risks additional errors to CopyErrorData(). But
it's not entirely obvious that geterrcode() is intended for this:

 * This is only intended for use in error callback subroutines, since there
 * is no other place outside elog.c where the concept is meaningful.
 */

a PG_CATCH() block isn't really an error callback subroutine. But it should be
fine.

Greetings,

Andres Freund