plpgsql versus SPI plan abstraction

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: plpgsql versus SPI plan abstraction
Date: 2013-01-30 21:23:18
Message-ID: 28025.1359580998@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I looked into the odd behavior noted recently on pgsql-novice that
the error context stack reported by plpgsql could differ between
first and subsequent occurrences of the same error:
http://www.postgresql.org/message-id/26370.1358539743@sss.pgh.pa.us

This seems to be specific to errors that are detected at plan time for a
potentially-simple expression. The example uses "1/0" which throws an
error when eval_const_expressions tries to simplify it. From plpgsql's
viewpoint, the error happens when it tries to use GetCachedPlan() to get
a plan tree that it can check for simple-ness. In this situation, we
have not pushed _SPI_error_callback onto the error context stack, so the
line it might contribute to the context report doesn't show up.
However, exec_simple_check_plan is set up to mark the PLpgSQL_expr as
non-simple at the outset, so that when it loses control due to the
error, that's how the already-cached PLpgSQL_expr is marked. Thus, on a
subsequent execution, we don't go through there but just pass off
control to SPI_execute_plan --- and it *does* set up _SPI_error_callback
before calling GetCachedPlan(). So now you get the additional line of
context.

There doesn't seem to be a comparable failure mode before 9.2, because
in previous releases planning would always occur before we created a
CachedPlanSource at all; so the failure would leave plpgsql still
without a cached PLpgSQL_expr, and the behavior would be consistent
across tries.

My first thought about fixing this was to export _SPI_error_callback
so that plpgsql could push it onto the context stack before doing
GetCachedPlan. But that's just another piercing of the veil of
modularity. What seems like a better solution is to export a SPI
wrapper of GetCachedPlan() that pushes the callback locally. With a
bit more work (a wrapper to get the CachedPlanSource list) we could
also stop letting pl_exec.c #include spi_priv.h, which is surely a
modularity disaster from the outset.

Does anyone see a problem with back-patching such a fix into 9.2,
so as to get rid of the context stack instability there?

BTW, I'm also wondering if it's really necessary for plpython/plpy_spi.c
to be looking into spi_priv.h ...

regards, tom lane

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kevin Grittner 2013-01-30 21:28:36 Re: autovacuum not prioritising for-wraparound tables
Previous Message Jim Nasby 2013-01-30 21:11:11 Re: Hm, table constraints aren't so unique as all that