Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wBLN1-000xfX-20 for pgsql-hackers@arkaria.postgresql.org; Fri, 10 Apr 2026 23:34:40 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wBLMy-00EqA6-2R for pgsql-hackers@arkaria.postgresql.org; Fri, 10 Apr 2026 23:34:37 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1wBLMy-00Eq9x-1H for pgsql-hackers@lists.postgresql.org; Fri, 10 Apr 2026 23:34:37 +0000 Received: from mail-yx1-xb12e.google.com ([2607:f8b0:4864:20::b12e]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wBLMx-00000000Qyl-121h for pgsql-hackers@postgresql.org; Fri, 10 Apr 2026 23:34:37 +0000 Received: by mail-yx1-xb12e.google.com with SMTP id 956f58d0204a3-6505ef94043so2513325d50.2 for ; Fri, 10 Apr 2026 16:34:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1775864073; cv=none; d=google.com; s=arc-20240605; b=Cyaw1M0QjuqEvyCmafELj/KuU8JxpaJQCqO8oHbbiw8wPW+jc8+GPcvejjzYfI73xp pCsTYu2jauP0u0gHv0RHyZQulbMbN0Rn/A80huzFuNQQyjEz7RNXe8ElXYGLRMX1LeoH 7dbMNm6w42tW0BtHZ3XPir7x+7Y5DgqPvdr3hVVIZ32kNoIzKEqiBadBvj4zCGi903Mx +SP9rwUYnsJ+TCislvdRJGUQGzQ1xbov38OHuL7C/x9RCaAOEEmvZRBS+6W0LYf0c0FK M38Z/2+N1E6m7K/8bottgpS/bnvhwcs4BB0mPxCLldnARpF31X1Z9v2y99gswzgiiXo8 6RfA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:dkim-signature; bh=UaqGWUw3uUnM+64Tg8nW/lXWQyWgh8E2EcFAWpB4n9c=; fh=9l7vhhxWTAxrfRloDuVhid+xBDvvdtgvNnDOeQr9+BI=; b=U/WvaG4LIAwzwkhh0m3IWpTnScU0fcEITpZP4762ZOdd6s4Bo5bAaAInUYwkzybyTV Rzgc5io8nZefNLY57oWwc/AaiMeEdNZ0e5g87ZiwOjDSH7aIQqsfF9/vY2wEuWEJOArM Rfyo3uDulBOB6UAJ752Xasj1raMLJFvZ6BpXYuHw/g+aQ27h7pzg8+EiRRV7w7pA9e+A /v8qGaonHHdIVnBlL0BgHg8nWLeQ5mh/n1a+jqsL1PMa4sRwOxTXBz7oeewEPwG3RVfm tLDt0d4EgYfzc2+PlfJSJ1KIvpzbOvs/q49pMwuHJ248ErN9jXWv1ea+hR+aetMPMV3M mhLw==; darn=postgresql.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1775864073; x=1776468873; darn=postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=UaqGWUw3uUnM+64Tg8nW/lXWQyWgh8E2EcFAWpB4n9c=; b=ASRGjoeRUhGmxnA1kDY0QKrp66e83nqFXcZbiyr3cgOM/T0KFcvo09KhlUZ1G/QgPn WdZPsVPGH4nRCqBKfE8KhM51m1C/V9bBjWSLopVrL0jcVW/vQGAmUYVk8R7OkDZ9+jmw QFQKr4z7ib6Qcdw8+ZrLrkJsajNApOabobCMCncvt4c41BY8A+0hgQD6s77ua3xlXqu3 NKe6cp9hs1d0ZYaXThxLaeYaKcoxNTC4Zb1NvC7SSJarJLa/u/ymJ6MBqAnL6FTnb0vF QJFNN/phFyxZSw3xs+o93kjk8M7RX4NcRCrkj+aEdiVYYfQEI/ir+tdYTcIoUWcDYblQ wC7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775864073; x=1776468873; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=UaqGWUw3uUnM+64Tg8nW/lXWQyWgh8E2EcFAWpB4n9c=; b=AwurAjOpYPZwRsAkp74nFEnbZJW2TBEsVx5JWERBpx9nm/6YxxersvykSDPb+Vv1gG RQ1cROG1VRhI7Xd9OCWVfEN6YS7gtR6BbyBufZAE/0IxkSspjiz8TMIMCOPS5yKhlZoE /uwmKUXeOJKet++ZsvprIULYEOEk1Zmny6cv3pj5L8YR944PMGyW1XCUIslkiJc7stux /DZu7ihHolI2a8zWXK9UDbaHa212BZDbasVnMuTpkuDKC/6BrqLEXfyl9RQY/p5Sd0dz 6sRg7TEe5TwfTinlcCLU8xvA53xaa+YAUhmyR9cUPdqErAz/fjsthCTqDxmKQTPToJSB i/Ag== X-Forwarded-Encrypted: i=1; AJvYcCUcIMvBt1OAoHiJdRcLUAxUTUOrToeBX3tSgkymvSlnm8MA4ykYKjejlG3FS0KbqCBnxUx8IKt//sygoqeF@postgresql.org X-Gm-Message-State: AOJu0Yy2eCFYvFSx6KkDUNFTJAA4MDawyujFo+/jG0heesE1CXcw7Sk1 GbT9xrxY3VfosI5tV9czHvm0HWE+ZMlUSZQmX0qRqs/yTZ6s+voPKbQFq9w9LgAAOtqkeTkkGJc CrHfIXfrwgcvoX3f/ya+dqTXVtszs4TE= X-Gm-Gg: AeBDievKxRyIv8XV7rLOnk05NYdzUxfTu/Mwf0B1d6NGfzpypxtRF7ENDspI7Z0Q7aC H2luyhbteslhZHVJhOttxiAsu7hv+xylpIDYA8GEvEjxlhcRVnklLaGWBMN87PJXcRjwmax6TUt /W+1cHZsueVX82THg8uFS4iBkc4fAoB5JpysJvFa5zTTI+V03PIhVE4qu9WkGh8ghNUMFNTBV1O cf/36ot6YHN7N2gKvpM6MSWnE3AQlR529LHKN7uQELhCIkPWoI0lT/XtCG4Cn2qmfoM/g93Az59 QFzqljcnTnNaK9cJaGwzJbYTPODogKH8BNloS3SmU9erC0a7/8g= X-Received: by 2002:a05:690e:2108:b0:650:3307:f7f1 with SMTP id 956f58d0204a3-65198aacebamr3784530d50.26.1775864073232; Fri, 10 Apr 2026 16:34:33 -0700 (PDT) MIME-Version: 1.0 References: <2BE661BA-D909-4093-BF78-DB9B0C099337@gmail.com> <77FA04FE-1F84-4DA1-8855-8BBFD8CC889A@gmail.com> <72AA2663-B642-4FB1-BDC2-5FAFF2D2DF15@gmail.com> <8561287B-19F1-421E-959D-99F4593CFF54@gmail.com> In-Reply-To: From: Haibo Yan Date: Fri, 10 Apr 2026 16:34:21 -0700 X-Gm-Features: AQROBzDQkV6eOT2LexqVNEXuyEVLcyhfl03O4uvRIjyNscahFjj_ycqylsg981w Message-ID: Subject: Re: Eliminating SPI / SQL from some RI triggers - take 3 To: Amit Langote Cc: Chao Li , Evan Montgomery-Recht , PostgreSQL-development Content-Type: multipart/alternative; boundary="0000000000004124ad064f239364" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000004124ad064f239364 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Apr 10, 2026 at 12:39=E2=80=AFAM Amit Langote wrote: > On Thu, Apr 9, 2026 at 7:26=E2=80=AFPM Chao Li w= rote: > > 0001 and 0002 look good to me. I didn=E2=80=99t review 0003 and don=E2= =80=99t 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 --0000000000004124ad064f239364 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Fri, Apr 10,= 2026 at 12:39=E2=80=AFAM Amit Langote <amitlangote09@gmail.com> wrote:
On Thu, Apr 9, 2026 at 7:26=E2=80=AFPM Ch= ao Li <li.ev= an.chao@gmail.com> wrote:
> 0001 and 0002 look good to me. I didn=E2=80=99t review 0003 and don=E2= =80=99t 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<= br> 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 h= ave a few comments:

    <= li>

    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 ch= eck for SPI_connect() failure. Since this m= odule is specifically meant to exercise ResourceOw= ner lifetime interactions, I think the helper itself should be robus= t in both success and error paths.

  1. Consider adding a follow-up test that does failure fi= rst, then success.

    That would help show that the helper does not leave a= ny lingering state behind after an error.

  2. Consider trimming the long explanatory comments in th= e regression test a bit.

    The rationale is useful, but some of it is repeated a= cross the commit message, the SQL file header, and the expected output.

Regards
Haibo
--0000000000004124ad064f239364--