Skip to content

New setting AllowUpdateOverwrite : 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

Merged
hvlad merged 3 commits into from
Apr 7, 2026
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 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
a7b3f39
Setting UpdateOverwriteMode (integer) renamed to AllowUpdateOverwrite…
hvlad Apr 6, 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
17 changes: 17 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,23 @@
#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.
#
# 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
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.

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.

AllowUpdateOverwriteTrigger or simple AllowUpdateOverwrite ?

Copy link
Copy Markdown
Member

@mrotteveel mrotteveel Apr 5, 2026

Choose a reason for hiding this comment

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

AllowUpdateOverwriteTrigger or simple AllowUpdateOverwrite ?

I assume you mean these in combination with an enum as Dmitry suggested. Then I'd suggest something like:

TriggerLostUpdateBehaviour = { Allow | Reject }

Copy link
Copy Markdown
Member Author

@hvlad hvlad Apr 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

AllowUpdateOverwriteTrigger or simple AllowUpdateOverwrite ?

I assume you mean these in combination with an enum as Dmitry suggested.

I meant it should be a boolean. I should have state it explicitly, sorry.

Then I'd suggest something like:

TriggerLostUpdateBehaviour = { Allow | Reject }

No, please. "Lost update" is well known term from another area - transaction's isolation and concurrency.
And there is no "lost update" in classical meaning here.

Copy link
Copy Markdown
Member

@dyemanov dyemanov Apr 6, 2026

Choose a reason for hiding this comment

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

AllowUpdateOverwriteTrigger or simple AllowUpdateOverwrite ?

Either AllowUpdateOverwrite[Trigger] (with default = true) or RejectUpdateOverwrite[Trigger] (with default = false) are fine with me.

Copy link
Copy Markdown
Member Author

@hvlad hvlad Apr 6, 2026

Choose a reason for hiding this comment

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

Ok, I'll change it by AllowUpdateOverwriteTrigger with default = true.

#
#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
87 changes: 62 additions & 25 deletions src/jrd/vio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6617,31 +6617,31 @@ static void refresh_fk_fields(thread_db* tdbb, Record* old_rec, record_param* cu
* 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 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() == old_rec->getFormat())
Comment thread
hvlad marked this conversation as resolved.
Outdated
{
if (memcmp(cur_rpb->rpb_address, old_rec->getData(), cur_rpb->rpb_length) == 0)
return;

fb_assert(overwriteMode == 1);
ERR_post(Arg::Gds(isc_random) << "UPDATE will overwrite changes made by trigger");
Comment thread
hvlad marked this conversation as resolved.
Outdated
}
// 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_random) << "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