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 1wAmaP-000K3E-0k for pgsql-hackers@arkaria.postgresql.org; Thu, 09 Apr 2026 10:26:09 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1wAmaM-0057ig-0s for pgsql-hackers@arkaria.postgresql.org; Thu, 09 Apr 2026 10:26:07 +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 1wAmaL-0057iX-2m for pgsql-hackers@lists.postgresql.org; Thu, 09 Apr 2026 10:26:06 +0000 Received: from mail-pl1-x62d.google.com ([2607:f8b0:4864:20::62d]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1wAmaK-00000000A7H-1AmG for pgsql-hackers@postgresql.org; Thu, 09 Apr 2026 10:26:06 +0000 Received: by mail-pl1-x62d.google.com with SMTP id d9443c01a7336-2ad21f437eeso3958185ad.0 for ; Thu, 09 Apr 2026 03:26:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1775730362; x=1776335162; 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=DvI+nod4y1QU95boYzbkeUatm6uoDxi+OO+2gcKb8aU=; b=TRTKahRtZy8/8IHogw97C5IsMKgUnjlrUM3699IC6Poo1lhDLWFQxgXeTcKyN1qEKz 9XgRZ7Y5AMt0JN0S6/WaLuly6UKG9DBL10w5w7LtWIM/2BAcb+qWe6i8oQxVUEC1Oyot 9gEjDBBHjaqU3IQlM+kOYJRnXi7zfQXagRelsXADusVi1MJ1UTPqqYzCWGPLamHM39d8 v09ZiRVdpXGOufHelq5lDywYKfdAXcaEeEAGwFBXwvhZoBJyuPoD6xXAZ7MuJpucMWln fPXJAOpCm0msWPR3ZmXyvd8HmtKxZohQJNMWm1haltQ7XweKN/YCNBxRSs+LHFbRd0Y+ Oq8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775730362; x=1776335162; 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=DvI+nod4y1QU95boYzbkeUatm6uoDxi+OO+2gcKb8aU=; b=Xsfp/3RqAqYI8poPg5yLAtnNrd8Do0UWCjFfCTa/RpdT70t+Q6Ahl+HXmRttDRjjiH LvCHpsvYVtd1N7Ed7SFBlbzy+90LefORFxtMpWFV9tatGQf64Xjxj+wUdYdKKaw8yXsp L3+FnSbYOGIWFuCi2BhwANsjIHO811DGqmXoXZ/jyjzCQn4d7iGeYhalkuqogAUUGz+X RcpXDIUx4kToCi8ug5qVjQhPA0JYuA+qflJR2ae7tZvNrsQtOQPI6gULJ/aluQ5h3Bjq zkSCrlmQu+dcdmaAgsKiLHBcBMoaONzkVXCCpB2Gq5t5uObt3nGdgQML/04Dt/Nr+aSw al/g== X-Forwarded-Encrypted: i=1; AJvYcCVsvUY14ua4QIRE+U0YLs5T1/so6/iPZ9k4qFU/prS9fdbFRtXeiGfL2LllGVmChsnnkNlf7Xd/BhZVSwYs@postgresql.org X-Gm-Message-State: AOJu0YzkVp7xr7Ek4s0bfr8ubUeDb/2yF5D1I5jcul405YGJIdsRnCf7 ZU1cJsh5uhfaMkuilP4zlgIF80u8xggXUNmE3HPN3wvMOIKh9tKtM2AK X-Gm-Gg: AeBDieseSwp26fmjd08Jry90t4qJmFCBQyHHmwIpqd2okukv7Svw15V9SZxCQx/3Dgo dkZQCKyFhfkSaUc1PgIZLRuYA5PJsLT6ZMD7Oaggk+DV9SQt7I063RYot11i4JewdsUhjxaiqmm iD15httH2DksDhqWdJLiyIKLdJ2u3rYZg7IF8tIS02gOHV7BL2o8zT1h/+bt0k6SSKCYVf4ogYr r96o5ctKajPPQNBEBdBuFguQ+eH1VYd57tGJX31UHTgybXby0Vi/Spr4EUSHkMiDDBaJY2abi/C NIf+QC/6LMriCjUzZO3UQVlt6oLRVqB4mMKb2VYEtb4+XdX/ERaHA80Wr2Mo5DKtpMyN29ZeIt0 NCNaGHdDhdp0Sw064vjpztapte7LlUvUEHY1HhDaeBjp7f7tTdD85cjYdc8+1Ob6Uga7B8RMOyK CqePMhCdNb//eIuBW9LV9kFmzCXLlGaEA= X-Received: by 2002:a17:903:32c6:b0:2b0:ac1e:9730 with SMTP id d9443c01a7336-2b2c7347516mr27425155ad.14.1775730361483; Thu, 09 Apr 2026 03:26:01 -0700 (PDT) Received: from smtpclient.apple ([45.32.121.103]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2b27472d1a2sm248482695ad.7.2026.04.09.03.25.58 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 Apr 2026 03:26:00 -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 18:25:21 +0800 Cc: Evan Montgomery-Recht , PostgreSQL-development Content-Transfer-Encoding: quoted-printable Message-Id: 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> 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 9, 2026, at 16:40, Amit Langote = wrote: >=20 > Hi, >=20 > On Thu, Apr 9, 2026 at 4:40=E2=80=AFPM Chao Li = wrote: >>> On Apr 8, 2026, at 22:26, Amit Langote = wrote: >>> 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. >> A few comments on v3: >=20 > Thanks for the review. >=20 >> 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); >> ``` >>=20 >> 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. >=20 > I'm dropping 0002 or have merged 0004 into it so this memory context > switch is no longer present. >=20 >> 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. >>=20 >> 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. >>=20 >> =46rom this perspective, I would assume a callback should not be = allowed to register a new callback. Can you please help confirm? >=20 > Good point on the re-entrant registration concern. I've added a > firing_batch_callbacks flag to AfterTriggersData that prevents > callbacks from registering new callbacks during > FireAfterTriggerBatchCallbacks(), with an Assert in > RegisterAfterTriggerBatchCallback() to enforce it. That should keep > the list being iterated from being modified. >=20 > The attached patches are updated accordingly. 0001 is the main fix > incorporating the per-query-level storage design, the transaction > boundary cleanup, and the firing_batch_callbacks guard. 0002 is a > followup that moves afterTriggerFiringDepth into AfterTriggersData as > a minor cleanup of 5c54c3ed1b9. Barring further feedback I plan to > commit 0001 and 0002 shortly. For 0003, I need to check on the policy > around adding new test modules during feature freeze before committing > it. >=20 > --=20 > Thanks, Amit Langote > = 0001 and 0002 look good to me. I didn=E2=80=99t review 0003 and don=E2=80=99= t intend to review it. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/