From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Andrew Dunstan <andrew@dunslane.net>,
        Alvaro Herrera <alvherre@alvh.no-ip.org>,
        Joe Conway <mail@joeconway.com>, Robert Haas <robertmhaas@gmail.com>,
        Corey Huinker <corey.huinker@gmail.com>,
        Nikita Glukhov <n.gluhov@postgrespro.ru>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Error-safe user functions
In-reply-to: <20221206005607.lxcrazckoalptwds@alap3.anarazel.de>
References: <20221205174231.6wpmbuxafnfzlbuh@alvherre.pgsql>
 <85e40849-4458-bbb9-62ee-d9e7faf0c7dc@dunslane.net>
 <3323802.1670263204@sss.pgh.pa.us> <3496391.1670276406@sss.pgh.pa.us>
 <20221205234734.sab3fuxy6q2sokjd@alap3.anarazel.de>
 <3518534.1670285891@sss.pgh.pa.us>
 <20221206005607.lxcrazckoalptwds@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Mon, 05 Dec 2022 16:56:07 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3523581.1670288815.1@sss.pgh.pa.us>
Date: Mon, 05 Dec 2022 20:06:55 -0500
Message-ID: <3523582.1670288815@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3523582.1670288815%40sss.pgh.pa.us>
Precedence: bulk

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