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