From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-committers@lists.postgresql.org
Subject: pgsql: Repair incorrect handling of AfterTriggerSharedData.ats_modified
Date: Wed, 22 Jan 2025 16:58:41 +0000
Message-ID: <E1tae3t-002wS7-CE@gemulon.postgresql.org> (raw)
Repair incorrect handling of AfterTriggerSharedData.ats_modifiedcols.
This patch fixes two distinct errors that both ultimately trace
to commit 71d60e2aa, which added the ats_modifiedcols field.
The more severe error is that ats_modifiedcols wasn't accounted for
in afterTriggerAddEvent's scanning loop that looks for a pre-existing
duplicate AfterTriggerSharedData. Thus, a new event could be
incorrectly matched to an AfterTriggerSharedData that has a different
value of ats_modifiedcols, resulting in the wrong tg_updatedcols
bitmap getting passed to the trigger whenever it finally gets fired.
We'd not noticed because (a) few triggers consult tg_updatedcols,
and (b) we had no tests exercising a case where such a trigger was
called as an AFTER trigger. In the test case added by this commit,
contrib/lo's trigger fails to remove a large object when expected
because (without this fix) it thinks the LO OID column hasn't changed.
The other problem was introduced by commit ce5aaea8c, which copied the
modified-columns bitmap into trigger-related storage. It made a copy
for every trigger event, whereas what we really want is to make a new
copy only when we make a new AfterTriggerSharedData entry. (We could
imagine adding extra logic to reduce the number of bitmap copies still
more, but it doesn't look worthwhile at the moment.) In a simple test
of an UPDATE of 10000000 rows with a single AFTER trigger, this thinko
roughly tripled the amount of memory consumed by the pending-triggers
data structures, from 160446744 to 480443440 bytes.
Fixing the first problem requires introducing a bms_equal() call into
afterTriggerAddEvent's scanning loop, which is slightly annoying from
a speed perspective. However, getting rid of the excessive bms_copy()
calls from the second problem balances that out; overall speed of
trigger operations is the same or slightly better, in my tests.
Discussion: https://postgr.es/m/3496294.1737501591@sss.pgh.pa.us
Backpatch-through: 13
Branch
------
REL_13_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/001c09d8b72ee67ea670a1e9a534fa96948829dd
Modified Files
--------------
contrib/lo/expected/lo.out | 69 ++++++++++++++++++++++++++++++++++++++++++
contrib/lo/sql/lo.sql | 40 ++++++++++++++++++++++++
src/backend/commands/trigger.c | 18 +++++------
3 files changed, 117 insertions(+), 10 deletions(-)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Repair incorrect handling of AfterTriggerSharedData.ats_modified
In-Reply-To: <E1tae3t-002wS7-CE@gemulon.postgresql.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox