Merge pull request #8957 from FirebirdSQL/work/UpdateOverwriteMode

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

Author: hvlad

builds/install/misc/firebird.conf

@@ -523,6 +523,31 @@
 #SubQueryConversion = false
 
 
+# ----------------------------
+# Defines how UPDATE should handle the case when a record was already updated by 
+# trigger or by the another positional UPDATE in the same cursor.
+#
+# 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.
+#
+# Setting AllowUpdateOverwrite manage how engine should handle such cases:
+# - if set to true (default), prior changes will be overwritten,
+# - if set to false, error "UPDATE will overwrite changes made by the trigger or 
+#	by the another UPDATE in the same cursor" will be raised.
+#
+# CAUTION!
+# There is no guarantee that this setting will be available in future Firebird
+# versions.
+#
+# Per-database configurable.
+#
+# Type: integer
+#
+#AllowUpdateOverwrite = true
+
+
 # ============================
 # Plugin settings
 # ============================

src/common/config/config.h

@@ -194,6 +194,7 @@ enum ConfigKey
 	KEY_OPTIMIZE_FOR_FIRST_ROWS,
 	KEY_OUTER_JOIN_CONVERSION,
 	KEY_SUBQUERY_CONVERSION,
+	KEY_ALLOW_UPDATE_OVERWRITE,
 	MAX_CONFIG_KEY		// keep it last
 };
 
@@ -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_BOOLEAN,	"AllowUpdateOverwrite",		false,	true}
 };
 
 
@@ -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_BOOL(getAllowUpdateOverwrite, KEY_ALLOW_UPDATE_OVERWRITE);
 };
 
 // Implementation of interface to access master configuration file

src/include/firebird/impl/msg/jrd.h

@@ -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, -901, "27", "000", "UPDATE will overwrite changes made by the trigger or by the another UPDATE in the same cursor")

src/jrd/vio.cpp

@@ -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);
@@ -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
@@ -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 AllowUpdateOverwrite is set to false, 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 allowOverwrite = dbb->dbb_config->getAllowUpdateOverwrite();
+
 	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;
@@ -6663,26 +6663,61 @@ static void refresh_fk_fields(thread_db* tdbb, Record* old_rec, record_param* cu
 	}
 
 	if (fields.isEmpty())
-		return;
+	{
+		if (allowOverwrite)
+			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;
+
+			ERR_post(Arg::Gds(isc_update_overwrite));		// UPDATE will overwrite changes made by the trigger or by another UPDATE in the same cursor
+		}
+		// 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 (allowOverwrite)
+				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) ||
+				(flag_cur && flag_old && MOV_compare(tdbb, &dsc_old, &dsc_cur) != 0))
+			{
+				// Record was modified by trigger.
+				ERR_post(Arg::Gds(isc_update_overwrite));		// UPDATE will overwrite changes made by the trigger or by another UPDATE in the same cursor
+			}
+		}
+		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++;
 		}
 	}
 }