Re: Problem with dblink

Joe Conway <mail@joeconway.com> writes:
> Tom Lane wrote:
> (More generally, I wonder if AtEOXact_SPI oughtn't be fixed to emit
> a WARNING if the SPI stack isn't empty, except in the error case.
> Neglecting SPI_finish is analogous to a resource leak, and we have
> code in place to warn about other sorts of leaks.)

> Is the attached what you had in mind?

Approximately.  A couple minor stylistic gripes:

1. AFAIR, all the other at-end-of-xact routines that take a flag telling
them if it's commit or abort define the flag as isCommit.  Having the
reverse convention for this one routine is confusing and a recipe for
errors, so please make it be isCommit too.

2. The initial comment for AtEOXact_SPI:

>    * Clean up SPI state at transaction commit or abort (we don't care which).

is now incorrect and needs to be updated (at least get rid of the
parenthetical note).

> !     if (_SPI_stack != NULL)
> !     {
>           free(_SPI_stack);
> +         if (!isAbort)
> +             ereport(WARNING,
> +                     (errcode(ERRCODE_WARNING),
> +                      errmsg("freeing non-empty SPI stack"),
> +                      errhint("Check for missing \"SPI_finish\" calls")));
> +     }
> +

While this isn't necessarily wrong, what would probably be a stronger
test is to complain if either _SPI_connected or _SPI_curid is not -1.
For one thing that would catch missing SPI_pop().  (Jan, any comment
about that?)

            regards, tom lane