BUG #7881: SQL function failures in long-lived calling contexts

[a***@tao11.riddles.org.uk]

The following bug has been logged on the website:

Bug reference: 7881
Logged by: Andrew Gierth
Email address: ***@tao11.riddles.org.uk
PostgreSQL version: 9.2.3
Operating system: any
Description:

The range type code accepts SQL functions for subtype_diff, but stores the
flinfo in a long-lived context (typcache).

The SQL function handler, fmgr_sql, isn't prepared to deal with the
possibility that the fcache entry may be left over from a previous query
that failed.

The combination of these two allows a non-superuser to provoke at least an
assertion failure as follows:

create or replace function inet_subdiff(inet,inet) returns float8 language
sql immutable as $f$ select ($2 - $1)::float8; $f$;

create type inetrange as range (subtype = inet, subtype_diff =
inet_subdiff);

create table inetr as select
format('[%s::,%s::]',to_hex(i),to_hex(i+1))::inetrange as r from
generate_series(0,65534) i;

postgres=# create index inetr_idx on inetr using gist (r);
ERROR: result is out of range
CONTEXT: SQL function "inet_subdiff" statement 1

postgres=# create index inetr_idx on inetr using gist (r);
TRAP: FailedAssertion("!(snapshot->regd_count > 0)", File: "snapmgr.c",
Line: 557)

I'm inclined to think this is fmgr_sql's fault for apparently assuming that
if an error is thrown that it'll never see the fcache entry again, but in
this example that's clearly not true.

[Tom Lane]

Post by a***@tao11.riddles.org.uk
The SQL function handler, fmgr_sql, isn't prepared to deal with the
possibility that the fcache entry may be left over from a previous query
that failed.

Yeah. The immediate cause of that failure is that it supposes that it's
continuing execution of the SQL function, when actually all the
underlying executor state was lost in the transaction abort. But it's
not just the execution state that can't be trusted anymore: the parsed
and planned query trees are not really safe either, since we no longer
hold any locks on underlying relations.

I've thought for some time that we should throw away much of functions.c
and rewrite the code to use the plancache to hold the function queries.
This'd not only make it more robust but allow function query plans to be
saved and reused across multiple queries. However, that's a large
change that probably couldn't be back-patched, even if someone had time
to write it right now.

If we just wanted to close off the crash risk, we could do it with a
localized patch by checking whether the fn_mcxt is a child of
CurTransactionContext, and throwing a "not supported" error if not.
Given the lack of prior complaints, this might be good enough for now.
However, I'm worried that people might actively be using SQL functions
in contexts like your example --- it would appear to work as long as the
functions didn't touch any relations and didn't suffer any errors.
So a patch like that might break applications that had been getting
along well enough so far.

Or we could add fields recording the current transaction/subtransaction
IDs, then throw away and regenerate the function cache entry if that
subxact is no longer active. This would be a bit more invasive/riskier
than throwing a "not supported" error, but it would preserve the
functionality.

I'm inclined to go for the third alternative, but does anyone think
we should just do "not supported" and call it a day?

regards, tom lane

[Tom Lane]

Post by Tom Lane
Or we could add fields recording the current transaction/subtransaction
IDs, then throw away and regenerate the function cache entry if that
subxact is no longer active. This would be a bit more invasive/riskier
than throwing a "not supported" error, but it would preserve the
functionality.

The attached patch seems fairly reasonable for this.

regards, tom lane