From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Andrew Dunstan <andrew@dunslane.net>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Joe Conway <mail@joeconway.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Corey Huinker <corey.huinker@gmail.com>
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Error-safe user functions
Date: Mon, 05 Dec 2022 20:06:55 -0500
Message-ID: <3523582.1670288815@sss.pgh.pa.us> (raw)
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, andrew@dunslane.net, alvherre@alvh.no-ip.org, mail@joeconway.com, robertmhaas@gmail.com, corey.huinker@gmail.com, n.gluhov@postgrespro.ru, pgsql-hackers@lists.postgresql.org
Subject: Re: Error-safe user functions
In-Reply-To: <3523582.1670288815@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox