Thread: missing "SPI_finish" that isn't missing

I probably just need to learn the right coding pattern for this.
What is it?

What I want this code to do:

a) If there is a table 'mytable', set theSavedStuff to a pstrdup'd in  TopMemoryContext copy of column s from one row
ifpresent.
 

b) In case of an error because there is no table 'mytable', just swallow  the error and do nothing. That's an expected
case.

c) In case of any other error (something unexpected), rethrow.

Case (a) works. Case (c) works. Case (b) works but produces this:

WARNING:  01000: transaction left non-empty SPI stack
HINT:  Check for missing "SPI_finish" calls.

I can verify that my SPI_finish call is being reached. In fact, in
case b, it returns SPI_ERROR_UNCONNECTED, as if even before my
PG_CATCH received control, the SPI "connection" got closed. But the
"SPI stack" still appears as if something is open?

In the context where this is called, there is no SPI connection already
open (if I try SPI_push_conditional at the start, it returns false).

I'm sure the only bug is in my understanding of how to approach this.
Can someone point out what I'm missing?

Thanks,
-Chap

{   MemoryContext curr;   SPI_connect();   curr = CurrentMemoryContext;   PG_TRY();   {       if ( SPI_OK_SELECT ==
SPI_execute(          "SELECT s FROM mytable", true, 1) && 1 == SPI_processed )       {
MemoryContextSwitchTo(TopMemoryContext);          theSavedStuff = (char const *)SPI_getvalue(
SPI_tuptable->vals[0],SPI_tuptable->tupdesc, 1);           MemoryContextSwitchTo(curr);       }   }   PG_CATCH();   {
   if ( ERRCODE_UNDEFINED_TABLE != geterrcode() )           PG_RE_THROW();       MemoryContextSwitchTo(curr);
FlushErrorState();  }   PG_END_TRY();   SPI_finish();
 
}

Chapman Flack <chap@anastigmatix.net> writes:
> I probably just need to learn the right coding pattern for this.
> What is it?

You can't just ignore a thrown error like that.  What you'd have to do
to make this coding pattern work is to set up a subtransaction, and either
commit it in the success path or roll it back upon catching an error.
(This is not terribly well documented, but the exception-block handling
in plpgsql provides a working model to follow.)  Without subtransaction
cleanup, lots of stuff will be left in incorrect states --- the unbalanced
SPI stack you're seeing is just the tip of the iceberg.

99% of the time, the path of least resistance at the C-code level is to
avoid expected error conditions in the first place, rather than try to
catch and clean up.  In this example, there are plenty of ways you might
test whether "mytable" exists before risking the SPI_execute call.
        regards, tom lane

On 12/24/15 16:37, Tom Lane wrote:

> to make this coding pattern work is to set up a subtransaction, and either
> commit it in the success path or roll it back upon catching an error.
> (This is not terribly well documented, but the exception-block handling
> in plpgsql provides a working model to follow.)

Ok, I've just had a look at that in plpgsql, and I see what you mean about
the path of least resistance ... though it's not completely obvious to me
how many pieces of that are there for other plpgsql purposes, and how many
are inherently necessary just to be able to catch and recover from an
error thrown from SPI.

BeginInternalSubTransaction? CreateExecutorState? CreateExprContext?
xact callback? subxact callback?

> 99% of the time, the path of least resistance at the C-code level is to
> avoid expected error conditions in the first place, rather than try to
> catch and clean up.  In this example, there are plenty of ways you might
> test whether "mytable" exists before risking the SPI_execute call.

I can think of:

- another SPI query "select 1 from pg_catalog.pg_class join pg_catalog.pg_namespace n on relnamespace = n.oid where
nspname= 'myschema' and relname = 'mytable'"
 

- InvalidOid != get_relname_relid("mytable",   GetSysCacheOid1(NAMESPACENAME, CStringGetDatum("myschema")))

- ... other variations on those ...

The first one strikes me funny because it's an even heavier SQL query
to plan and execute just to find out if I can execute the original
trivial one. (Which doesn't really matter, admittedly, since I need to
do it exactly once.) The second one strikes me funny for the mixture of API
levels, undocumented low level to check existence followed by documented
SPI level for the original query. Almost tempts me to ditch SPI entirely
and learn how to use heap_* methods for the single trivial row retrieval
I need, only by the time I've figured out all that, I'll have forgotten
what I was trying to do in the first place.

Is there a particular, reasonably tidy idiom that has emerged as the usual,
customary approach to a task like this?

Thanks,
-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> Is there a particular, reasonably tidy idiom that has emerged as the usual,
> customary approach to a task like this?

You could construct a RangeVar node and then use RangeVarGetRelid,
but I dunno that that's really any cleaner than inquiring about it
using the syscaches.  The syscache APIs are really quite unlikely
to change in any meaningful way, if only because there's too much
code depending on them.
        regards, tom lane