MIME-Version: 1.0
References: <20250331212648.ad4ab804559001d7f0788741@sraoss.co.jp>
 <CAOVWO5q8cfbHiZuL-wSxknnriPCBYpqRCnQKfH38WuLwFuxkSw@mail.gmail.com>
 <20260327005646.8b9df4658b2dd510d3f95ec0@sraoss.co.jp>
 <CA+HiwqGFuhuSus8L7O36CYoeeZtGJ4PO0cR94qKDbtMKR4T2OA@mail.gmail.com>
 <20260327120057.04d16c1240fe402a3a093ada@sraoss.co.jp>
In-Reply-To: <20260327120057.04d16c1240fe402a3a093ada@sraoss.co.jp>
From: Amit Langote <amitlangote09@gmail.com>
Date: Fri, 27 Mar 2026 14:36:21 +0900
Message-ID: 
 <CA+HiwqF3RXW7BGv2PTs0Q73CJfUDseQSc=+hUsk==R64Uva87A@mail.gmail.com>
Subject: Re: Add comments about fire_triggers argument in ri_triggers.c
To: Yugo Nagata <nagata@sraoss.co.jp>
Cc: surya poondla <suryapoondla4@gmail.com>, pgsql-hackers@postgresql.org,
	Kirill Reshke <reshkekirill@gmail.com>
Content-Type: multipart/mixed; boundary="0000000000008d4fdf064dfae26b"
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BHiwqF3RXW7BGv2PTs0Q73CJfUDseQSc%3D%2BhUsk%3D%3DR64Uva87A%40mail.gmail.com>
Precedence: bulk

--0000000000008d4fdf064dfae26b
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 27, 2026 at 12:01=E2=80=AFPM Yugo Nagata <nagata@sraoss.co.jp> =
wrote:
> On Fri, 27 Mar 2026 09:39:17 +0900
> Amit Langote <amitlangote09@gmail.com> wrote:
>
> > On Fri, Mar 27, 2026 at 12:56=E2=80=AFAM Yugo Nagata <nagata@sraoss.co.=
jp> wrote:
> > >
> > > Hi,
> > >
> > > Thank you all for the review and comments.
> > >
> > > > Yes Amit, I agree that SPI_execute_snapshot() comments do provide s=
ome
> > > > context on AFTER triggers, but I still feel the newly added comment
> > > > in ri_PerformCheck() gives additional context on why the fire_trigg=
ers is
> > > > set to false.
> > >
> > > Yes, that is what I intended. The existing comments on
> > > SPI_execute_snapshot() explain how the fire_triggers parameter works,
> > > but I would like to add a comment explaining why the AFTER trigger fo=
r
> > > RI needs to set it to false.
> > >
> > > If the explanation of the effect of fire_triggers seems redundant, I =
am
> > > fine with the following shorter version:
> > >
> > > +        * Set fire_triggers to false to ensure that check triggers f=
ire after all
> > > +        * RI updates on the same row are complete.
> >
> > Thanks for the updated patch.  Yes, adding the comment might be good,
> > but I'd suggest a small tweak:
> >
> > +        * Set fire_triggers to false to ensure that AFTER triggers
> > are queued in
> > +        * the outer query's after-trigger context and fire after all
> > RI updates on
> > +        * the same row are complete, rather than immediately.
> >
> > Two changes:
> >
> > * "check triggers" -> "AFTER triggers", since fire_triggers=3Dfalse
> > affects any AFTER triggers queued during the SPI execution, not just
> > RI check triggers.
> >
> > * mention of the outer query's after-trigger context to explain the
> > mechanism by which the deferral works.
> >
> > Does that additional context help?
>
> Thank you for the suggestion.
> That looks good to me. It is clearer than the previous version.

Ok, will push the attached.

--=20
Thanks, Amit Langote

--0000000000008d4fdf064dfae26b
Content-Type: application/octet-stream;
	name="v3-0001-Add-comment-explaining-fire_triggers-false-in-ri_.patch"
Content-Disposition: attachment;
	filename="v3-0001-Add-comment-explaining-fire_triggers-false-in-ri_.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_mn8gxidq0>
X-Attachment-Id: f_mn8gxidq0

RnJvbSAwOGM5MDk5N2I2ODJkODhkNTlkNjQ4MWVhMDA1NDIxMDhiODY3YTU5IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBBbWl0IExhbmdvdGUgPGFtaXRsYW5AcG9zdGdyZXNxbC5vcmc+
CkRhdGU6IEZyaSwgMjcgTWFyIDIwMjYgMTQ6MzM6MzUgKzA5MDAKU3ViamVjdDogW1BBVENIIHYz
XSBBZGQgY29tbWVudCBleHBsYWluaW5nIGZpcmVfdHJpZ2dlcnM9ZmFsc2UgaW4KIHJpX1BlcmZv
cm1DaGVjaygpCgpUaGUgcmVhc29uIGZvciBwYXNzaW5nIGZpcmVfdHJpZ2dlcnM9ZmFsc2UgdG8g
U1BJX2V4ZWN1dGVfc25hcHNob3QoKQppbiByaV9QZXJmb3JtQ2hlY2soKSB3YXMgbm90IGRvY3Vt
ZW50ZWQsIG1ha2luZyBpdCB1bmNsZWFyIHdoeSBpdCB3YXMKZG9uZSB0aGF0IHdheS4gIEFkZCBh
IGNvbW1lbnQgZXhwbGFpbmluZyB0aGF0IGl0IGVuc3VyZXMgQUZURVIKdHJpZ2dlcnMgYXJlIHF1
ZXVlZCBpbiB0aGUgb3V0ZXIgcXVlcnkncyBhZnRlci10cmlnZ2VyIGNvbnRleHQgYW5kCmZpcmUg
b25seSBhZnRlciBhbGwgUkkgdXBkYXRlcyBvbiB0aGUgc2FtZSByb3cgYXJlIGNvbXBsZXRlLgoK
QXV0aG9yOiBZdWdvIE5hZ2F0YSA8bmFnYXRhQHNyYW9zcy5jby5qcD4KUmV2aWV3ZWQtYnk6IEtp
cmlsbCBSZXNoa2UgPHJlc2hrZWtpcmlsbEBnbWFpbC5jb20+ClJldmlld2VkLWJ5OiBTdXJ5YSBQ
b29uZGxhIDxzdXJ5YXBvb25kbGE0QGdtYWlsLmNvbT4KRGlzY3Vzc2lvbjogaHR0cHM6Ly9wb3N0
Z3IuZXMvbS8yMDI1MDMzMTIxMjY0OC5hZDRhYjgwNDU1OTAwMWQ3ZjA3ODg3NDFAc3Jhb3NzLmNv
LmpwCi0tLQogc3JjL2JhY2tlbmQvdXRpbHMvYWR0L3JpX3RyaWdnZXJzLmMgfCA4ICsrKysrKyst
CiAxIGZpbGUgY2hhbmdlZCwgNyBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCgpkaWZmIC0t
Z2l0IGEvc3JjL2JhY2tlbmQvdXRpbHMvYWR0L3JpX3RyaWdnZXJzLmMgYi9zcmMvYmFja2VuZC91
dGlscy9hZHQvcmlfdHJpZ2dlcnMuYwppbmRleCBkMjJiOGVmN2YzYy4uNjIzMGEyZWE5YWQgMTAw
NjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxzL2FkdC9yaV90cmlnZ2Vycy5jCisrKyBiL3NyYy9i
YWNrZW5kL3V0aWxzL2FkdC9yaV90cmlnZ2Vycy5jCkBAIC0yNTgyLDcgKzI1ODIsMTMgQEAgcmlf
UGVyZm9ybUNoZWNrKGNvbnN0IFJJX0NvbnN0cmFpbnRJbmZvICpyaWluZm8sCiAJCQkJCQkgICBz
YXZlX3NlY19jb250ZXh0IHwgU0VDVVJJVFlfTE9DQUxfVVNFUklEX0NIQU5HRSB8CiAJCQkJCQkg
ICBTRUNVUklUWV9OT0ZPUkNFX1JMUyk7CiAKLQkvKiBGaW5hbGx5IHdlIGNhbiBydW4gdGhlIHF1
ZXJ5LiAqLworCS8qCisJICogRmluYWxseSB3ZSBjYW4gcnVuIHRoZSBxdWVyeS4KKwkgKgorCSAq
IFNldCBmaXJlX3RyaWdnZXJzIHRvIGZhbHNlIHRvIGVuc3VyZSB0aGF0IEFGVEVSIHRyaWdnZXJz
IGFyZSBxdWV1ZWQgaW4KKwkgKiB0aGUgb3V0ZXIgcXVlcnkncyBhZnRlci10cmlnZ2VyIGNvbnRl
eHQgYW5kIGZpcmUgYWZ0ZXIgYWxsIFJJIHVwZGF0ZXMKKwkgKiBvbiB0aGUgc2FtZSByb3cgYXJl
IGNvbXBsZXRlLCByYXRoZXIgdGhhbiBpbW1lZGlhdGVseS4KKwkgKi8KIAlzcGlfcmVzdWx0ID0g
U1BJX2V4ZWN1dGVfc25hcHNob3QocXBsYW4sCiAJCQkJCQkJCQkgIHZhbHMsIG51bGxzLAogCQkJ
CQkJCQkJICB0ZXN0X3NuYXBzaG90LCBjcm9zc2NoZWNrX3NuYXBzaG90LAotLSAKMi40Ny4zCgo=
--0000000000008d4fdf064dfae26b--