Re: Error-safe user functions - Mailing list pgsql-hackers

Andres Freund <andres@anarazel.de> writes:
> On 2022-12-05 19:18:11 -0500, Tom Lane wrote:
>> but I'm not sure that's much of an improvement.  Note that it'd
>> *not* be correct to declare it as anything more specific than Node*,
>> since the fmgr context pointer is Node* and we're not expecting
>> callers to do their own IsA checks to see what they were passed.

> Ah - I hadn't actually grokked that that's the reason for the
> void*. Unless I missed a comment to that regard, entirely possible, it
> seems worth explaining that above errsave_start().

There's a comment about that in elog.h IIRC, but no harm in saying
it in elog.c as well.

Having said that, I am warming a little bit to making these pointers
be Node* or an alias spelling of that rather than void*.

>> I don't think refactoring to remove the duplication would improve it.

> Why? I think a populate_edata() or such seems to make sense. And the
> required argument to skip ->backtrace and error_context_stack processing
> seem like things that'd be good to document anyway.

Meh.  Well, I'll have a look, but it seems kind of orthogonal to the
main point of the patch.

>> Hmm, maybe.  It would be a bigger change from existing code, but
>> I don't think very many call sites would be impacted.  (But by
>> the same token, we'd not save much code this way.)  Personally
>> I put more value on keeping similar APIs between InputFunctionCall
>> and InputFunctionCallSafe, but I won't argue hard if you're insistent.

> I think it's good to diverge from the existing code, because imo the
> behaviour is quite different and omitting the SAFE_ERROR_OCCURRED()
> check will lead to brokenness.

True, but it only helps for the immediate caller of InputFunctionCallSafe,
not for call levels further out.  Still, I'll give that a look.

>> I wasn't trying all that hard on the error tests, because I think
>> 0003 is just throwaway code at this point.

> I am mainly interested in having *something* test erroring out hard when
> using the "Safe" mechanism, which afaict we don't have with the patches
> as they stand.  You're right that it'd be better to do that without COPY
> in the way, but it doesn't seem all that crucial.

Hmm, either I'm confused or you're stating that backwards --- aren't
the hard-error code paths already tested by our existing tests?

> But perhaps it's even worth having such a function properly exposed:
> It's not at all rare to parse text data during ETL and quite often
> erroring out fatally is undesirable. As savepoints are undesirable
> overhead-wise, there's a lot of SQL out there that tries to do a
> pre-check about whether some text could be cast to some other data
> type. A function that'd try to cast input to a certain type without
> erroring out hard would be quite useful for that.

Corey and Vik are already talking about a non-error CAST variant.
Maybe we should leave this in abeyance until something shows up
for that?  Otherwise we'll be making a nonstandard API for what
will probably ultimately be SQL-spec functionality.  I don't mind
that as regression-test infrastructure, but I'm a bit less excited
about exposing it as a user feature.

            regards, tom lane