Thread: Assertion failure in plpgsql with INSTEAD OF rule
This test case:
CREATE TABLE atable(n int);
CREATE TABLE btable(n int);
CREATE RULE insteadrule AS ON INSERT TO atable DO INSTEAD delete from
btable;
CREATE FUNCTION rulecrash() RETURNS void AS $$
begin insert into atable values(1);
end;
$$ LANGUAGE plpgsql;
SELECT rulecrash();
Fails an assertion on versions 8.2, 8.3 and CVS HEAD:
TRAP: FailedAssertion("!(stmt->mod_stmt)", File: "pl_exec.c", Line: 2800)
The assertion is in exec_stmt_execsql():
> rc = SPI_execute_plan(expr->plan, values, nulls,
> estate->readonly_func, tcount);
>
> /*
> * Check for error, and set FOUND if appropriate (for historical reasons
> * we set FOUND only for certain query types). Also Assert that we
> * identified the statement type the same as SPI did.
> */
> switch (rc)
> {
> case SPI_OK_SELECT:
> Assert(!stmt->mod_stmt);
> exec_set_found(estate, (SPI_processed != 0));
> break;
>
> case SPI_OK_INSERT:
> case SPI_OK_UPDATE:
> case SPI_OK_DELETE:
> case SPI_OK_INSERT_RETURNING:
> case SPI_OK_UPDATE_RETURNING:
> case SPI_OK_DELETE_RETURNING:
>>> Assert(stmt->mod_stmt);
> exec_set_found(estate, (SPI_processed != 0));
> break;
>
> case SPI_OK_SELINTO:
> case SPI_OK_UTILITY:
> Assert(!stmt->mod_stmt);
> break;
>
> default:
> elog(ERROR, "SPI_execute_plan failed executing query \"%s\": %s",
> expr->query, SPI_result_code_string(rc));
> }
The problem is that mod_stmt is determined for the query that has
canSetTag set, but in case of an INSTEAD OF rule that rewrites the
statement into a different command, an INSERT into a DELETE in this
case, canSetTag is not set. The return code of SPI_execute_plan still
indicates SPI_OK_DELETE, so we have a mismatch in what that assertion is
trying to check.
mod_stmt is used to control whether to throw an error if the query
returns more than one row and there's an INTO, and ISTM the logic is
correct for that use. However, the logic for when to set FOUND is
different, so I think the correct fix is to simply remove the assertion.
At least for back-branches; you could argue for changing the behavior of
FOUND, but that could break existing applications.
Barring flaws in my diagnosis, I'm going to remove those Assertions.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> The problem is that mod_stmt is determined for the query that has
> canSetTag set, but in case of an INSTEAD OF rule that rewrites the
> statement into a different command, an INSERT into a DELETE in this
> case, canSetTag is not set. The return code of SPI_execute_plan still
> indicates SPI_OK_DELETE, so we have a mismatch in what that assertion is
> trying to check.
> mod_stmt is used to control whether to throw an error if the query
> returns more than one row and there's an INTO, and ISTM the logic is
> correct for that use. However, the logic for when to set FOUND is
> different, so I think the correct fix is to simply remove the assertion.
> At least for back-branches; you could argue for changing the behavior of
> FOUND, but that could break existing applications.
I think the real problem is this bit at the tail end of _SPI_execute_plan:
/* * If none of the queries had canSetTag, we return the last query's result * code, but not its auxiliary
results(for backwards compatibility). */ if (my_res == 0) my_res = res;
This always seemed a bit dubious to me, and in the light of this example
I wonder whether it was even really backwards-compatible. The problem
of course is what to return instead --- it almost seems like we'd have
to invent a new SPI return code "SPI_OK_REWRITTEN" or some such.
In any case, having an INSERT set FOUND on the basis of a rewritten
DELETE seems 100% wrong to me. It won't even be self-consistent because
the actual value of SPI_processed won't be coming from the DELETE.
And what if it's rewritten into a utility statement (eg NOTIFY)?
regards, tom lane
Tom Lane wrote: > Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: >> The problem is that mod_stmt is determined for the query that has >> canSetTag set, but in case of an INSTEAD OF rule that rewrites the >> statement into a different command, an INSERT into a DELETE in this >> case, canSetTag is not set. The return code of SPI_execute_plan still >> indicates SPI_OK_DELETE, so we have a mismatch in what that assertion is >> trying to check. > >> mod_stmt is used to control whether to throw an error if the query >> returns more than one row and there's an INTO, and ISTM the logic is >> correct for that use. However, the logic for when to set FOUND is >> different, so I think the correct fix is to simply remove the assertion. >> At least for back-branches; you could argue for changing the behavior of >> FOUND, but that could break existing applications. > > I think the real problem is this bit at the tail end of _SPI_execute_plan: > > /* > * If none of the queries had canSetTag, we return the last query's result > * code, but not its auxiliary results (for backwards compatibility). > */ > if (my_res == 0) > my_res = res; > > This always seemed a bit dubious to me, and in the light of this example > I wonder whether it was even really backwards-compatible. The problem > of course is what to return instead --- it almost seems like we'd have > to invent a new SPI return code "SPI_OK_REWRITTEN" or some such. > > In any case, having an INSERT set FOUND on the basis of a rewritten > DELETE seems 100% wrong to me. It won't even be self-consistent because > the actual value of SPI_processed won't be coming from the DELETE. > And what if it's rewritten into a utility statement (eg NOTIFY)? If _SPI_execute_plan reaches the above "if" and sets "my_res = res", SPI_processed is left at 0. So FOUND is always set to false if the rewritten command type doesn't match the original. Note that even when an INSERT is rewritten into another INSERT, there's actually no guarantee that SPI_processed means what the caller would think it means. It could be rewritten into an INSERT of a different table, a log table, for example. I don't much like the idea of leaving FOUND untouched either. If there's an earlier query in the function that sets FOUND, and you have an "IF FOUND ..." block after an INSERT that follows, you might not realize that FOUND is still left at the value set by the earlier query. Hmm, perhaps FOUND should be set to NULL in this case, to mean "unknown". -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> If _SPI_execute_plan reaches the above "if" and sets "my_res = res",
> SPI_processed is left at 0. So FOUND is always set to false if the
> rewritten command type doesn't match the original.
Right, and I think that's probably sane behavior. The original command
got rewritten to nothing, therefore it processed zero rows. I'd be
happy with code that guaranteed that FOUND got set false in that case.
The point I'm making is that the code doesn't guarantee it now (and
would not do so after removing those Asserts either). Consider
create rule r1 as on insert to foo do instead notify foo_insert;
As implemented, this would send us through the SPI_OK_UTILITY path
and nothing happens to FOUND. I say that's a bug.
So what I'm thinking is:
1. We can't redefine the SPI interface in back branches, so there's
probably little alternative but to remove those Asserts there.
2. In HEAD, I think we should have SPI return a new SPI_OK_REWRITTEN
code for this case, and have plpgsql respond to that by always setting
found = false. With that, the Asserts can stay (in fact the new path
should assert !mod_stmt, since it shouldn't have found any canSetTag
query).
regards, tom lane
Tom Lane wrote: > So what I'm thinking is: > > 1. We can't redefine the SPI interface in back branches, so there's > probably little alternative but to remove those Asserts there. Committed this. > 2. In HEAD, I think we should have SPI return a new SPI_OK_REWRITTEN > code for this case, and have plpgsql respond to that by always setting > found = false. With that, the Asserts can stay (in fact the new path > should assert !mod_stmt, since it shouldn't have found any canSetTag > query). I'll look into this as a separate patch. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com