Skip to content

New setting UpdateOverwriteMode : defines how UPDATE should handle the case when a record was updated by trigger. #8957

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hvlad wants to merge 2 commits into
base: v5.0-release
Choose a base branch
from
+97 −30
Open

New setting UpdateOverwriteMode : defines how UPDATE should handle the case when a record was updated by trigger. #8957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97f703a
New setting UpdateOverwriteMode : defines how UPDATE should handle th…
hvlad Mar 26, 2026
fe0cc14
Follow some suggestions by @dyemanov
hvlad Apr 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions builds/install/misc/firebird.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,27 @@
#SubQueryConversion = false


# ----------------------------
# Defines how UPDATE should handle the case when a record was updated by trigger.
#
# It is possible that BEFORE or AFTER UPDATE trigger was fired previously by the
# current UPDATE statement for the other record and has already changed the
# current record being updated. Due to cursor stability, UPDATE should not see
# such changes, and could silently overwrite them.
#
# If set to 0 (default), trigger changes will be overwritten by the UPDATE.
# If set to 1, error "UPDATE will overwrite changes made by trigger" will be raised.
#
# CAUTION!
# There is no guarantee that this setting will be available in future Firebird
# versions.
#
# Per-database configurable.
#
# Type: integer
Copy link
Copy Markdown
Member

@dyemanov dyemanov Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why integer and 0/1 instead of boolean and false/true? And rename to something like UpdateOverwriteStrictMode or RestrictUpdateOverwrite?

Copy link
Copy Markdown
Member Author

@hvlad hvlad Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I thought about 3rd value - to issue warning, not error. But it was not implemented as I found it unpractical, iirc.

As for the setting name - I can agree with anything reasonable ;) Lets decide if we need such changes.

Copy link
Copy Markdown
Member Author

@hvlad hvlad Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I going to add forgoten note that this setting is temporary and could be removed in future versions.

Copy link
Copy Markdown
Member

@dyemanov dyemanov Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the setting name - I can agree with anything reasonable ;)

I suppose we need more opinions here. @mrotteveel , can you suggest any better naming for a boolean version of the switch?

Copy link
Copy Markdown
Contributor

@aafemt aafemt Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "DisableTableMutation"?

Copy link
Copy Markdown
Contributor

@aafemt aafemt Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be made fully consistent without re-implementing of per-row triggers mechanics.

Can the logic from MERGE statement that prevents double update of the same record be used here? It would at least make throwing of the error consistent.

Copy link
Copy Markdown
Member Author

@hvlad hvlad Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There no what is generally accepted as "update conflict".

But there's something quite similar to the commonly known "lost update", so maybe [Reject|Prevent|Disable]TriggerLostUpdate ?

I read it as "trigger that lost some update" which is obviously not correct ;)

Can you explain, please, what is bad with UpdateOverwriteMode ? Don't you like Mode ?
Here we have UPDATE that really can OVERWRITE other change and we introduce MODE that allows or not allows it.

Copy link
Copy Markdown
Member Author

@hvlad hvlad Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be made fully consistent without re-implementing of per-row triggers mechanics.

Can the logic from MERGE statement that prevents double update of the same record be used here? It would at least make throwing of the error consistent.

I'm not sure we should prevent this as it can disable perfectly valid actions of AFTER UPDATE triggers.

Copy link
Copy Markdown
Contributor

@aafemt aafemt Apr 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One would argue that these actions are not valid (and were known to cause problems in the past, including already mentioned "lost update"). As a workaround the check can be disarmed for AFTER triggers.

Advantage of this solution would be that the implementation is simple: just go through savepoints and throw the error if any (except transaction's one) already have the bit for the record set.

PS: The setting in this case can be named like "PreventDoubleUpdate" with values "Always", "Never" and "NotInAfterTriggers" with "Always" as default. ;-)

Copy link
Copy Markdown
Member

@dyemanov dyemanov Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain, please, what is bad with UpdateOverwriteMode ? Don't you like Mode ?

Exactly. Mode suggests more options than just true/false, or maybe two options but explicitly named -- like enum UpdateOverwriteMode = { Ignore , Restrict }. This is why I initially suggested StrictMode or alike, because it converts Mode into a boolean.

#
#UpdateOverwriteMode = 0

# ============================
# Plugin settings
# ============================
Expand Down
3 changes: 3 additions & 0 deletions src/common/config/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,9 @@ void Config::checkValues()

checkIntForLoBound(KEY_PARALLEL_WORKERS, 1, true);
checkIntForHiBound(KEY_PARALLEL_WORKERS, values[KEY_MAX_PARALLEL_WORKERS].intVal, false);

checkIntForLoBound(KEY_UPDATE_OVERWRITE_MODE, 0, true);
checkIntForHiBound(KEY_UPDATE_OVERWRITE_MODE, 1, false);
}


Expand Down
6 changes: 5 additions & 1 deletion src/common/config/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ enum ConfigKey
KEY_OPTIMIZE_FOR_FIRST_ROWS,
KEY_OUTER_JOIN_CONVERSION,
KEY_SUBQUERY_CONVERSION,
KEY_UPDATE_OVERWRITE_MODE,
MAX_CONFIG_KEY // keep it last
};

Expand Down Expand Up @@ -314,7 +315,8 @@ constexpr ConfigEntry entries[MAX_CONFIG_KEY] =
{TYPE_INTEGER, "MaxParallelWorkers", true, 1},
{TYPE_BOOLEAN, "OptimizeForFirstRows", false, false},
{TYPE_BOOLEAN, "OuterJoinConversion", false, true},
{TYPE_BOOLEAN, "SubQueryConversion", false, false}
{TYPE_BOOLEAN, "SubQueryConversion", false, false},
{TYPE_INTEGER, "UpdateOverwriteMode", false, 0}
};


Expand Down Expand Up @@ -652,6 +654,8 @@ class Config : public RefCounted, public GlobalStorage
CONFIG_GET_PER_DB_BOOL(getOuterJoinConversion, KEY_OUTER_JOIN_CONVERSION);

CONFIG_GET_PER_DB_BOOL(getSubQueryConversion, KEY_SUBQUERY_CONVERSION);

CONFIG_GET_PER_DB_INT(getUpdateOverwriteMode, KEY_UPDATE_OVERWRITE_MODE);
};

// Implementation of interface to access master configuration file
Expand Down
2 changes: 2 additions & 0 deletions src/include/firebird/impl/msg/jrd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,3 +975,5 @@ FB_IMPL_MSG(JRD, 990, sweep_read_only, -901, "42", "000", "Database in read only
FB_IMPL_MSG(JRD, 991, sweep_attach_no_cleanup, -901, "42", "000", "Attachment has no cleanup flag set")
// Codes 992..997 are used in v6
FB_IMPL_MSG(JRD, 998, no_user_att_while_restore, -901, "HY", "000", "User attachments are not allowed for the database being restored")
// Codes 999..1004 are used in v6
FB_IMPL_MSG(JRD, 1005, update_overwrite_trigger, -901, "27", "000", "UPDATE will overwrite changes made by trigger")
95 changes: 66 additions & 29 deletions src/jrd/vio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ static void protect_system_table_delupd(thread_db* tdbb, const jrd_rel* relation
bool force_flag = false);
static void purge(thread_db*, record_param*);
static void replace_record(thread_db*, record_param*, PageStack*, const jrd_tra*);
static void refresh_fk_fields(thread_db*, Record*, record_param*, record_param*);
static void refresh_changed_fields(thread_db*, Record*, record_param*, record_param*);
static SSHORT set_metadata_id(thread_db*, Record*, USHORT, drq_type_t, const char*);
static void set_nbackup_id(thread_db*, Record*, USHORT, drq_type_t, const char*);
static void set_owner_name(thread_db*, Record*, USHORT);
Expand Down Expand Up @@ -3329,7 +3329,7 @@ bool VIO_modify(thread_db* tdbb, record_param* org_rpb, record_param* new_rpb, j
fb_assert(!(org_rpb->rpb_runtime_flags & RPB_undo_read));

if (undo_read)
refresh_fk_fields(tdbb, old_record, org_rpb, new_rpb);
refresh_changed_fields(tdbb, old_record, org_rpb, new_rpb);
}

// If we're the system transaction, modify stuff in place. This saves
Expand Down Expand Up @@ -6605,43 +6605,43 @@ static void replace_record(thread_db* tdbb,
}


static void refresh_fk_fields(thread_db* tdbb, Record* old_rec, record_param* cur_rpb,
static void refresh_changed_fields(thread_db* tdbb, Record* old_rec, record_param* cur_rpb,
record_param* new_rpb)
{
/**************************************
*
* r e f r e s h _ f k _ f i e l d s
* r e f r e s h _ c h a n g e d _ f i e l d s
*
**************************************
*
* Functional description
* Update new_rpb with foreign key fields values changed by cascade triggers.
* Consider self-referenced foreign keys only.
* Also, if UpdateOverwriteMode is set to 1, raise error when non self-referenced
* foreign key fields were changed by user triggers.
*
* old_rec - old record before modify
* cur_rpb - just read record with possibly changed FK fields
* cur_rpb - just read record with possibly changed fields
* new_rpb - new record evaluated by modify statement and before-triggers
*
**************************************/
const Database* dbb = tdbb->getDatabase();
const auto overwriteMode = dbb->dbb_config->getUpdateOverwriteMode();

jrd_rel* relation = cur_rpb->rpb_relation;

MET_scan_partners(tdbb, relation);

if (!(relation->rel_foreign_refs.frgn_relations))
return;

const FB_SIZE_T frgnCount = relation->rel_foreign_refs.frgn_relations->count();
if (!frgnCount)
return;
const FB_SIZE_T frgnCount = relation->rel_foreign_refs.frgn_relations ?
relation->rel_foreign_refs.frgn_relations->count() : 0;

RelationPages* relPages = cur_rpb->rpb_relation->getPages(tdbb);

// Collect all fields of all foreign keys
// Collect all fields of self-referenced foreign keys
SortedArray<int, InlineStorage<int, 16> > fields;

for (FB_SIZE_T i = 0; i < frgnCount; i++)
{
// We need self-referenced FK's only
if ((*relation->rel_foreign_refs.frgn_relations)[i] == relation->rel_id)
{
index_desc idx;
Expand All @@ -6663,26 +6663,63 @@ static void refresh_fk_fields(thread_db* tdbb, Record* old_rec, record_param* cu
}

if (fields.isEmpty())
return;
{
if (overwriteMode == 0)
return;

DSC desc1, desc2;
for (FB_SIZE_T idx = 0; idx < fields.getCount(); idx++)
if (cur_rpb->rpb_record->getFormat()->fmt_version == old_rec->getFormat()->fmt_version)
{
if (memcmp(cur_rpb->rpb_address, old_rec->getData(), cur_rpb->rpb_length) == 0)
return;

fb_assert(overwriteMode == 1);
ERR_post(Arg::Gds(isc_update_overwrite_trigger)); // UPDATE will overwrite changes made by trigger
}
// Else compare field-by-field
}

for (FB_SIZE_T fld = 0, frn = 0; fld < relation->rel_current_format->fmt_count; fld++)
{
// Detect if user changed FK field by himself.
const int fld = fields[idx];
const bool flag_old = EVL_field(relation, old_rec, fld, &desc1);
const bool flag_new = EVL_field(relation, new_rpb->rpb_record, fld, &desc2);
dsc dsc_old;
const bool flag_old = EVL_field(relation, old_rec, fld, &dsc_old);

// If field was not changed by user - pick up possible modification by
// system cascade trigger
if (flag_old == flag_new &&
(!flag_old || (flag_old && !MOV_compare(tdbb, &desc1, &desc2))))
const bool is_fk = (frn < fields.getCount() && fields[frn] == fld);
if (!is_fk)
{
const bool flag_tmp = EVL_field(relation, cur_rpb->rpb_record, fld, &desc1);
if (flag_tmp)
MOV_move(tdbb, &desc1, &desc2);
else
new_rpb->rpb_record->setNull(fld);
if (overwriteMode == 0)
continue;

dsc dsc_cur;
const bool flag_cur = EVL_field(relation, cur_rpb->rpb_record, fld, &dsc_cur);

// Check if current record differs from old record
if ((flag_cur != flag_old) ||
Copy link
Copy Markdown
Member

@dyemanov dyemanov Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the field didn't exist in the old format but exists in the new format and gets some value updated by a trigger (e.g. because the new field is NOT NULL), an error will be raised. Is it intended?

Copy link
Copy Markdown
Member Author

@hvlad hvlad Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any field of current record was changed by trigger, that fired for another record - yes, this is error.
Despite of formats.

(flag_cur && flag_old && MOV_compare(tdbb, &dsc_old, &dsc_cur) != 0))
{
// Record was modified by trigger.
fb_assert(overwriteMode == 1);
ERR_post(Arg::Gds(isc_update_overwrite_trigger)); // UPDATE will overwrite changes made by trigger
}
}
else
{
dsc dsc_new;
const bool flag_new = EVL_field(relation, new_rpb->rpb_record, fld, &dsc_new);

// If field was not changed by user - pick up possible modification by
// system cascade trigger
if (flag_old == flag_new &&
(!flag_old || (flag_old && !MOV_compare(tdbb, &dsc_old, &dsc_new))))
{
dsc dsc_cur;
const bool flag_cur = EVL_field(relation, cur_rpb->rpb_record, fld, &dsc_cur);
if (flag_cur)
MOV_move(tdbb, &dsc_cur, &dsc_new);
else
new_rpb->rpb_record->setNull(fld);
}

frn++;
}
}
}
Expand Down
Loading