Re: Eliminating SPI / SQL from some RI triggers - take 3

On Sat, Apr 11, 2026 at 8:34 AM Haibo Yan <tristan.yim@gmail.com> wrote:
> On Fri, Apr 10, 2026 at 12:39 AM Amit Langote <amitlangote09@gmail.com> wrote:
>> Here's the remaning patch to add src/test/modules/test_spi_resowner
>> rebased against master. I'm holding off on committing the test module
>> until I confirm the policy on new test modules during feature freeze.
>> It's also worth discussing whether this is the right place for testing
>> C extensions that use SPI with a dedicated resource owner, or whether
>> that coverage belongs elsewhere.
>
> I reviewed the patch, and overall it looks close. I have a few comments:
>
> Should spi_exec_sql() be made exception-safe?
>
> The current implementation does not restore CurrentResourceOwner or release/delete childowner on all error paths, and it also does not check for SPI_connect() failure. Since this module is specifically meant to exercise ResourceOwner lifetime interactions, I think the helper itself should be robust in both success and error paths.
>
> Consider adding a follow-up test that does failure first, then success.
>
> That would help show that the helper does not leave any lingering state behind after an error.
>
> Consider trimming the long explanatory comments in the regression test a bit.
>
> The rationale is useful, but some of it is repeated across the commit message, the SQL file header, and the expected output.

Thanks Haibo for the review. Your points are well taken and would need
to be addressed if this module were to be committed, but I've been
reconsidering whether to commit it at all. It was written to reproduce
a specific crash caused by an extension's idiosyncratic use of SPI
with a dedicated resource owner, a pattern that's specific to PostGIS
and similar extensions rather than something core PostgreSQL
exercises. Now that the crash is fixed, the module's main value is as
a regression test for that one scenario. I'm not convinced it pulls
its weight as a permanent addition to the test suite, especially given
the maintenance burden and the time it adds to test runs.

I'll hold off on committing it unless someone feels strongly that it
should be included.

-- 
Thanks, Amit Langote