database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-07-27 12:41:57 +03:00
Code Activity

Repair incorrect handling of AfterTriggerSharedData.ats_modifiedcols.

Browse Source 
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
This commit is contained in:
Tom Lane
2025-01-22 11:58:20 -05:00
parent 20b4819d0e
commit 3085993052
3 changed files with 117 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
src/backend/commands/trigger.c
View File

@ -3723,13 +3723,6 @@ afterTriggerCopyBitmap(Bitmapset *src)
if (src == NULL)
return NULL;
/* Create event context if we didn't already */
if (afterTriggers.event_cxt == NULL)
afterTriggers.event_cxt =
AllocSetContextCreate(TopTransactionContext,
"AfterTriggerEvents",
ALLOCSET_DEFAULT_SIZES);
oldcxt = MemoryContextSwitchTo(afterTriggers.event_cxt);
dst = bms_copy(src);
@ -3831,16 +3824,21 @@ afterTriggerAddEvent(AfterTriggerEventList *events,
(char *) newshared >= chunk->endfree;
newshared--)
{
/* compare fields roughly by probability of them being different */
if (newshared->ats_tgoid == evtshared->ats_tgoid &&
newshared->ats_relid == evtshared->ats_relid &&
newshared->ats_event == evtshared->ats_event &&
newshared->ats_firing_id == 0 &&
newshared->ats_table == evtshared->ats_table &&
newshared->ats_firing_id == 0)
newshared->ats_relid == evtshared->ats_relid &&
bms_equal(newshared->ats_modifiedcols,
evtshared->ats_modifiedcols))
break;
}
if ((char *) newshared < chunk->endfree)
{
*newshared = *evtshared;
/* now we must make a suitably-long-lived copy of the bitmap */
newshared->ats_modifiedcols = afterTriggerCopyBitmap(evtshared->ats_modifiedcols);
newshared->ats_firing_id = 0; /* just to be sure */
chunk->endfree = (char *) newshared;
}
@ -5857,7 +5855,7 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
new_shared.ats_table = transition_capture->tcs_private;
else
new_shared.ats_table = NULL;
new_shared.ats_modifiedcols = afterTriggerCopyBitmap(modifiedCols);
new_shared.ats_modifiedcols = modifiedCols;
afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_depth].events,
&new_event, &new_shared);