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 1wAk08-000HLL-0B for pgsql-hackers@arkaria.postgresql.org; Thu, 09 Apr 2026 07:40:32 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wAk06-004Fha-0m for pgsql-hackers@arkaria.postgresql.org; Thu, 09 Apr 2026 07:40:31 +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 1wAk05-004FhP-2y for pgsql-hackers@lists.postgresql.org; Thu, 09 Apr 2026 07:40:30 +0000 Received: from mail-pf1-x429.google.com ([2607:f8b0:4864:20::429]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wAk04-000000008on-0f7z for pgsql-hackers@postgresql.org; Thu, 09 Apr 2026 07:40:30 +0000 Received: by mail-pf1-x429.google.com with SMTP id d2e1a72fcca58-82cf976ecacso265537b3a.1 for ; Thu, 09 Apr 2026 00:40:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1775720426; x=1776325226; darn=postgresql.org; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=7rxQMXk8wWCMt71n+1olL1U1O3nL2GBB7btvB9MwVYg=; b=bOgxz73V11zeqHViudstoemE/K30d71Za50gkTfMYzS1brP6TJItImrOmCHyvJQCqF vrwxJW4yKUAHi5DDgHj1vSyQ/Yvi1P1Agce9it15PJT15mQ7DJyc1DqlwvYWUmmp4kr+ LeUntTPYFF3UsqAT3sjy5HXDr4QV28uDTt2WsN1vHp2OlEOQ/Ry45GTtvy5EzEcGLovH 4tIbjysG9V8cauuVMWUh7kbMDcTXkNo8l3FnwBI7cVt9vO+w6AaxPODep4noskI7LXTs MkHAnnj4i6g6r+ur74hwMT2BwGAHjef5ZoeWcigtAjOFwi8Hf+t1SboNyYH8mx4Xmf/o MBTg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775720426; x=1776325226; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-gg:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=7rxQMXk8wWCMt71n+1olL1U1O3nL2GBB7btvB9MwVYg=; b=Rp9qlN7Tfxr/1mvnr3ikYyXYB3N0AM1D09cdpPlSP2dJsHVNK/eLMnYAJf9VaEC5J8 DQGfFXajYrBL0POjP8DrvugCqA5FpKGtTbgP7afqw8Bk6N/0Iq/Znym5h4+fPRLvTHSs HP7CU/+nwj2d8GjFiAGReZvjGqoqywWYXdMfW9wfx5x2APaa1561DSEND0yolh8l0gm0 E47p2eMW/BogI3VBGW5bVbl7jNU4k/TKg0eZuFA5kjZleADknmkd3qVBOcr1j9VjakMF C90/H44QV7/RoHXKsixo9RjwoIm5NLVPrH5l+PYA2YMVFFr3Q4sh0HDRxkZGkaDojXu/ tw/w== X-Forwarded-Encrypted: i=1; AJvYcCUIAwNEK+uaTE3iuizCcc5+YLcP08U+kH2NHKr69Boo33+9YbHoEA1xP6PYREXcXOmZ8sFi6YK8T5KCn1er@postgresql.org X-Gm-Message-State: AOJu0Yzn2iV41Hutq0iIT+Vx9oq4vWKw6as5bMulJCTkpgPe/ynf0ANw buFj2YiKhGGtqErR/6nZ+uTqoPsPK4Fq5T6zpp910oJfIzvhtka6RuFf X-Gm-Gg: AeBDiesMpUSym64LQEpQoLdoUNvTd4RJikCphrDLzxLxWh6dHdT5JulWlsjmXtqHcws 1z38r8YWbja9+IdWKeaLwEhiBfn4gxBtXCGzPIRQ0lQW9OgUnOQeCzzmXdrSYm6KD4AeS7nVCV6 Clx8542/Sq760uEsVd0ZUudK7z/+WnHOJDKkc/8y201p4YFO7Zt2mX9fNivOdmu/q0o5khXJ1DB Yj2fFwcTN6ZFStkNTFZzD3O8XzYD44JVL9rbM+KwvfKc2FYmdZZjHV0Fz0DRJUcea5Wtz7Sooc6 Ub9/9TBahEkSd8NQG9F64FqoqidMTZcQXwM55vwL922HCs/ZpoQK4+p1od+0juAUOmZza2eREb4 ePxi9PrkGRl/60YaSpCirP83Sr9Ii55kIaiOc7nCa1OB1NXfGGfRK/uhWzq2lxUlxjzSXMfNx4y o/zss8/dYPa7A36LaUXOU3jAjILJ+PNjw= X-Received: by 2002:a05:6a00:815:b0:82a:7734:8c6a with SMTP id d2e1a72fcca58-82dd8aeb11amr2562699b3a.48.1775720425513; Thu, 09 Apr 2026 00:40:25 -0700 (PDT) Received: from smtpclient.apple ([45.32.121.103]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-82cf9b21c92sm24896650b3a.11.2026.04.09.00.40.22 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 Apr 2026 00:40:24 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\)) Subject: Re: Eliminating SPI / SQL from some RI triggers - take 3 From: Chao Li In-Reply-To: Date: Thu, 9 Apr 2026 15:39:44 +0800 Cc: Evan Montgomery-Recht , PostgreSQL-development Content-Transfer-Encoding: quoted-printable Message-Id: <8561287B-19F1-421E-959D-99F4593CFF54@gmail.com> References: <2BE661BA-D909-4093-BF78-DB9B0C099337@gmail.com> <77FA04FE-1F84-4DA1-8855-8BBFD8CC889A@gmail.com> <72AA2663-B642-4FB1-BDC2-5FAFF2D2DF15@gmail.com> To: Amit Langote X-Mailer: Apple Mail (2.3864.400.21) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk > On Apr 8, 2026, at 22:26, Amit Langote = wrote: >=20 > On Wed, Apr 8, 2026 at 6:58=E2=80=AFPM Amit Langote = wrote: >> On Wed, Apr 8, 2026 at 10:23=E2=80=AFAM Amit Langote = wrote: >>> On Tue, Apr 7, 2026 at 10:00=E2=80=AFPM Evan Montgomery-Recht >>> wrote: >>>> The patch also adds a test module (test_spi_func) with a C function >>>> that executes SQL via SPI_connect/SPI_execute/SPI_finish, since = this >>>> crash cannot be triggered from PL/pgSQL. The test exercises the >>>> C-level SPI INSERT with multiple FK constraints, FK violations, and >>>> nested PL/pgSQL-calls-C-SPI (matching the PostGIS call pattern). >>=20 >> I applied only the test module changes and it passes (without >> crashing) even without your proposed fix. It seems that's because the >> C function in test_spi_func calling SPI is using the same resource >> owner as the parent SELECT. I think you'd need to create a resource >> owner manually in the spi_exec() C function to reproduce the crash, = as >> done in the attached 0001, which contains the src/test changes >> extracted from your patch modified as described, including renaming >> the C function to spi_exec_sql(). >>=20 >> Also, the test cases that call spi_exec() (_sql()) directly from a >> SELECT don't actually exercise the crash path because there is no >> outer trigger-firing loop active. query_depth is 0 inside the inner >> SPI's AfterTriggerEndQuery, so the old guard wouldn't suppress the >> callback there anyway. The critical case requires spi_exec_sql() to = be >> called from inside an AFTER trigger, where query_depth > 0 causes the >> guard to defer the callback past the inner resource owner's lifetime. >> I've added that test case. I kept your original test cases as they >> still provide useful coverage of C-level SPI FK behavior even if they >> don't exercise the crash path specifically. Maybe your original >> PostGIS test suite that hit the crash did have the right structure, >> but that's not reflected in the patch as far as I can tell. >>=20 >> I've also renamed the module to test_spi_resowner to better reflect >> what it's about. >>=20 >> For the fix, I have a different proposal. As you observed, the >> query_depth > 0 early return in FireAfterTriggerBatchCallbacks() = means >> that the nested SPI's callbacks get called under the outer resource >> owner, which may not be the same as the one that SPI used. I think it >> was a mistake to have that early return in the first place. Instead = we >> could remember for each callback what firing level it should be = called >> at, so the nested SPI's callbacks fire before returning to the parent >> level and parent-level callbacks fire when the parent level = completes. >> I have implemented that in the attached 0002 along with transaction >> boundary cleanup of callbacks, which passes the check-world for me, >> but I'll need to stare some more at it before committing. >>=20 >> Let me know if this also fixes your own in-house test suite or if you >> have any other suggestions or if you think I am missing something. >=20 > One more cleanup patch attached as 0003: afterTriggerFiringDepth was > added by commit 5c54c3ed1 as a file-static variable, which in > hindsight should have been a field in AfterTriggersData alongside the > other per-transaction after-trigger state. This patch makes that > correction. >=20 > One alternative design worth considering for 0002: storing > batch_callbacks per query level in AfterTriggersQueryData rather than > as a single list in AfterTriggersData, so callbacks naturally live at > the query level where they were registered and get cleaned up with > AfterTriggerFreeQuery on abort. Deferred constraints still need a > top-level list in AfterTriggersData since they fire outside any query > level. FireAfterTriggerBatchCallbacks() takes a list parameter and the > caller passes either the query-level or top-level list as appropriate. > This eliminates the need for firing_depth-matched firing entirely. I > did that in 0004. I think I like it over 0002. Will look more > closely tomorrow morning. >=20 >=20 > -- > Thanks, Amit Langote > = A few comments on v3: 1 - 0002 ``` static void FireAfterTriggerBatchCallbacks(void) { + List *remaining =3D NIL; + List *to_fire =3D NIL; ListCell *lc; =20 - if (afterTriggers.query_depth > 0) - return; + /* remaining and to_fire lists must survive until callbacks = complete */ + MemoryContext oldcxt =3D = MemoryContextSwitchTo(TopTransactionContext); ``` I think remaining and to_fire should stay in the same context of = afterTriggers.batch_callbacks, so instead of hard coding = TopTransactionContext, we can use = GetMemoryChunkContext(afterTriggers.batch_callbacks), which makes the = intention explicit. 2 - 0004, I noticed one potential problem, although I am not sure = whether it can really happen in practice. This version stores callback = items at the individual query depth, and = FireAfterTriggerBatchCallbacks() now iterates the callback list for that = depth and invokes each callback directly. My concern is that if one of = those callbacks needs to register a new callback, that would append a = new item to the same list while it is being iterated. That seems unsafe = to me, because list append may create a new list structure underneath. = If that happens, we may end up modifying the list being traversed, which = does not look safe. This problem doesn=E2=80=99t exist in 0002, because 0002 splits = afterTriggers.batch_callbacks into remaining and to_fire, and reset = afterTriggers.batch_callbacks =3D remaining before running callbacks. = But the problem is, if a callback registers a new callback, the new = callback goes to afterTriggers.batch_callbacks, so it won=E2=80=99t get = executed. =46rom this perspective, I would assume a callback should not be allowed = to register a new callback. Can you please help confirm? Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/