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

On Fri, Apr 10, 2026 at 12:39 AM Amit Langote <amitlangote09(at)gmail(dot)com>
wrote:

> On Thu, Apr 9, 2026 at 7:26 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> > 0001 and 0002 look good to me. I didn’t review 0003 and don’t intend to
> review it.
>
> I've now pushed 0001 (34a3078629) and 0002 (d6e96bacd3c).
>
> 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.
>
> --
> Thanks, Amit Langote
>

I reviewed the patch, and overall it looks close. I have a few comments:

1.

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.
2.

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.
3.

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.

Regards
Haibo