On Thu, Apr 9, 2026 at 7:26 PM Chao Li <li.evan.chao@gmail.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:
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.
