Thread: rule-related crash in v11
For reasons that I'm not quite sure about, the following test case
crashes in v11, but not earlier versions:
create table abc(n int);
create table xyz(n int);
create function fun() returns int as $$begin insert into abc values
(1); return 1; end$$ language plpgsql;
create or replace rule rule1 as on insert to abc do delete from xyz;
select fun();
create or replace rule rule1 as on insert to abc do instead delete from xyz;
select fun();
I get:
TRAP: FailedAssertion("!(!stmt->mod_stmt)", File: "pl_exec.c", Line: 4106)
The xyz table doesn't seem to be important; I can reproduce the crash
if I change 'delete from xyz' to 'do nothing' in both places. But
it's critical to 'SELET fun()' after the first CREATE OR REPLACE RULE
statement and before the second one. The INSERT inside the function
is also critical -- omitting that prevents the crash. I suspect the
problem is likely related to some of the changes made to spi.c rather
than to changes made on the plpgsql side of things, but that might be
wrong.
My colleague Tushar Ahuja deserves credit for finding this problem; I
can take credit only for modifying his test case to work against
unmodified PostgreSQL.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> For reasons that I'm not quite sure about, the following test case
> crashes in v11, but not earlier versions:
Crashes just fine in prior versions for me, at least as far back as 9.3,
but probably much further. Note that I was doing an extra select fun()
right after creating the function --- I don't think that should affect
the behavior, but maybe it does? Or maybe you were testing non-assert
builds?
The core problem here seems to be that exec_stmt_execsql sets mod_stmt
once when the query is first planned, and doesn't account for the idea
that the statement's classification might change later. But adding
(or removing) a DO INSTEAD rule can indeed change that.
Looking at what mod_stmt is used for, we've got
(1) the Assert that's crashing, and its siblings, which are just meant
to cross-check that mod_stmt is consistent with the SPI return code.
(2) two places that decide to enforce STRICT behavior if mod_stmt
is true.
I think we could just drop those Asserts. As for the other things,
maybe we should force STRICT on the basis of examining the raw
parsetree (which really is immutable) rather than what came out of
the planner. It doesn't seem like a great idea that INSERT ... INTO
should stop being considered strict if there's currently a rewrite
rule that changes it into something else.
regards, tom lane
On Fri, May 25, 2018 at 11:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> For reasons that I'm not quite sure about, the following test case >> crashes in v11, but not earlier versions: > > Crashes just fine in prior versions for me, at least as far back as 9.3, > but probably much further. Note that I was doing an extra select fun() > right after creating the function --- I don't think that should affect > the behavior, but maybe it does? Or maybe you were testing non-assert > builds? Oops, yeah, my back-branch builds were without assertions. > The core problem here seems to be that exec_stmt_execsql sets mod_stmt > once when the query is first planned, and doesn't account for the idea > that the statement's classification might change later. But adding > (or removing) a DO INSTEAD rule can indeed change that. > > Looking at what mod_stmt is used for, we've got > > (1) the Assert that's crashing, and its siblings, which are just meant > to cross-check that mod_stmt is consistent with the SPI return code. > > (2) two places that decide to enforce STRICT behavior if mod_stmt > is true. > > I think we could just drop those Asserts. As for the other things, > maybe we should force STRICT on the basis of examining the raw > parsetree (which really is immutable) rather than what came out of > the planner. It doesn't seem like a great idea that INSERT ... INTO > should stop being considered strict if there's currently a rewrite > rule that changes it into something else. Yes, that does sound like surprising behavior. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 25, 2018 at 11:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Looking at what mod_stmt is used for, we've got
>> (1) the Assert that's crashing, and its siblings, which are just meant
>> to cross-check that mod_stmt is consistent with the SPI return code.
>> (2) two places that decide to enforce STRICT behavior if mod_stmt
>> is true.
>>
>> I think we could just drop those Asserts. As for the other things,
>> maybe we should force STRICT on the basis of examining the raw
>> parsetree (which really is immutable) rather than what came out of
>> the planner. It doesn't seem like a great idea that INSERT ... INTO
>> should stop being considered strict if there's currently a rewrite
>> rule that changes it into something else.
> Yes, that does sound like surprising behavior.
On closer inspection, the specific Assert you're hitting is the only one
of the bunch that's really bogus. It's actually almost backwards: if we
have a statement that got rewritten into some other kind of statement by a
rule, it almost certainly *was* an INSERT/UPDATE/DELETE to start with.
But I think there are corner cases where spi.c can return SPI_OK_REWRITTEN
where we'd not have set mod_stmt (e.g., empty statement list), so I'm not
comfortable with asserting mod_stmt==true either. So the attached patch
just drops it.
Not sure if this is worth a regression test case.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 1eb421b..ef013bc 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_stmt_execsql(PLpgSQL_execstate *est
*** 4031,4049 ****
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
CachedPlanSource *plansource = (CachedPlanSource *) lfirst(l);
- ListCell *l2;
! foreach(l2, plansource->query_list)
{
! Query *q = lfirst_node(Query, l2);
!
! if (q->canSetTag)
! {
! if (q->commandType == CMD_INSERT ||
! q->commandType == CMD_UPDATE ||
! q->commandType == CMD_DELETE)
! stmt->mod_stmt = true;
! }
}
}
}
--- 4031,4050 ----
foreach(l, SPI_plan_get_plan_sources(expr->plan))
{
CachedPlanSource *plansource = (CachedPlanSource *) lfirst(l);
! /*
! * We could look at the raw_parse_tree, but it seems simpler to
! * check the command tag. Note we should *not* look at the Query
! * tree(s), since those are the result of rewriting and could have
! * been transmogrified into something else entirely.
! */
! if (plansource->commandTag &&
! (strcmp(plansource->commandTag, "INSERT") == 0 ||
! strcmp(plansource->commandTag, "UPDATE") == 0 ||
! strcmp(plansource->commandTag, "DELETE") == 0))
{
! stmt->mod_stmt = true;
! break;
}
}
}
*************** exec_stmt_execsql(PLpgSQL_execstate *est
*** 4108,4119 ****
break;
case SPI_OK_REWRITTEN:
- Assert(!stmt->mod_stmt);
/*
* The command was rewritten into another kind of command. It's
* not clear what FOUND would mean in that case (and SPI doesn't
! * return the row count either), so just set it to false.
*/
exec_set_found(estate, false);
break;
--- 4109,4120 ----
break;
case SPI_OK_REWRITTEN:
/*
* The command was rewritten into another kind of command. It's
* not clear what FOUND would mean in that case (and SPI doesn't
! * return the row count either), so just set it to false. Note
! * that we can't assert anything about mod_stmt here.
*/
exec_set_found(estate, false);
break;