Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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

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

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

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.

#
#UpdateOverwriteMode = 0

# ============================
# Plugin settings
# ============================
Expand Down
3 changes: 3 additions & 0 deletions src/common/config/config.cpp
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
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
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
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

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

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