New setting UpdateOverwriteMode : defines how UPDATE should handle the case when a record was updated by trigger.#8957
New setting UpdateOverwriteMode : defines how UPDATE should handle the case when a record was updated by trigger.#8957hvlad wants to merge 2 commits intov5.0-releasefrom
Conversation
…e case when a record was updated by trigger.
|
On 3/26/26 12:04, Vlad Khorsun wrote:
After #8538 <#8538>
there was long private discussion with customer (big sponsor of Firebird).
The root of the issue is the way Firebird implements per-row triggers
- it is far from SQL standard in regards of consistency.
There is well-known historical reasons for it and I don't want to
repeat it here.
By my opinion, the best way to fix the issue would be to re-implement
per-row triggers according to SQL standard - to fire before and after
whole statement procesing. But this is not quick nor easy task.
Finally this patch was implemented and customer uses it to prepare own
migration to v5.
Vlad, am I missing something or this patch works only with system
triggers that implement cascade FK?
|
The routine, I modified in the patch, was initially used to workaroud issue with self-referenced FK's |
dyemanov
left a comment
dyemanov
left a comment
There was a problem hiding this comment.
Honestly, I expected this PR to implement the same logic as seems to exist for FKs:
If field was not changed by user - pick up possible modification by trigger
instead of raising an error unconditionally.
However, AFAIU, we cannot detect this reliably at the VIO layer, because cannot distinguish between new.field explicitly assigned to the old value or copied from org_record. This would need accessing the ModifyNode's assignment list and checking for modified fields there. Although I'm not sure this is going to be as simple.
Did you consider this approach? Because the new mode reminds me the famous "table is mutating" error in Oracle, if raised even for non-conflicting changes.
| # | ||
| # Per-database configurable. | ||
| # | ||
| # Type: integer |
There was a problem hiding this comment.
Why integer and 0/1 instead of boolean and false/true? And rename to something like UpdateOverwriteStrictMode or RestrictUpdateOverwrite?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, I going to add forgoten note that this setting is temporary and could be removed in future versions.
src/jrd/vio.cpp
Outdated
| return; | ||
|
|
||
| fb_assert(overwriteMode == 1); | ||
| ERR_post(Arg::Gds(isc_random) << "UPDATE will overwrite changes made by trigger"); |
There was a problem hiding this comment.
Please add a new ISC code, unless this gonna be reimplemented in v6.
| 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) || |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If any field of current record was changed by trigger, that fired for another record - yes, this is error.
Despite of formats.
I consider this as wrong way because it let user triggers to implicitly change original UPDATE semantic. Case with self-referenced FK's is more predictable and consistent (unless same fields not changed by user trigger).
No. By the reasons as explained above.
The more I think on this issue, the more I agree with Oracle - it should not be allowed. And, actually, I've implemented such check when works on the issue past year. But - not a surprise - it was too restrictive and customer does not accepted it. Finally, the setting UpdateOverwriteMode was introduced and agreed as compromise solution. Ideally, we should reimplement the way per-row triggers works to provide consistensy and comply with SQL standard. |
3 participants
After #8538 there was long private discussion with customer (big sponsor of Firebird).
The root of the issue is the way Firebird implements per-row triggers - it is far from SQL standard in regards of consistency.
There is well-known historical reasons for it and I don't want to repeat it here.
By my opinion, the best way to fix the issue would be to re-implement per-row triggers according to SQL standard - to fire before and after whole statement processing. But this is not quick nor easy task.
Finally this patch was implemented and customer uses it to prepare own migration to v5.
Now it is time to decide if it could be accepted into codebase.