Re: rule-related crash in v11 - Mailing list pgsql-hackers

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;