From 5ad183eab1525a58a0bf20fb798fec8ef5d90893 Mon Sep 17 00:00:00 2001 From: jian he Date: Wed, 11 Dec 2024 14:45:52 +0800 Subject: [PATCH v10 1/1] overwritten virtual generated column value to null in trigger overwritten trigger assign values for virtual generated column to null. so virtual and stored generated column trigger bahavior is more aligned. --- doc/src/sgml/trigger.sgml | 2 +- src/backend/commands/trigger.c | 77 ++++++++++--------- src/pl/plpgsql/src/pl_exec.c | 4 +- .../regress/expected/generated_virtual.out | 21 ++--- src/test/regress/sql/generated_virtual.sql | 8 +- 5 files changed, 61 insertions(+), 51 deletions(-) diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml index 46c6c2f126..3a91eae156 100644 --- a/doc/src/sgml/trigger.sgml +++ b/doc/src/sgml/trigger.sgml @@ -287,7 +287,7 @@ higher-level programming language should prevent access to a stored generated column in the NEW row in a BEFORE trigger. Changes to the value of a generated - column in a BEFORE trigger are ignored and will be + column (stored or virtual) in a BEFORE trigger are ignored and will be overwritten. Virtual generated columns are never computed when triggers fire. In the C language interface, their content is undefined in a trigger function. diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 473541a2d2..d304ac8072 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -102,7 +102,6 @@ static void AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo, bool is_crosspart_update); static void AfterTriggerEnlargeQueryState(void); static bool before_stmt_triggers_fired(Oid relid, CmdType cmdType); -static void check_modified_virtual_generated(TupleDesc tupdesc, HeapTuple tuple); /* @@ -2506,7 +2505,26 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, } else if (newtuple != oldtuple) { - check_modified_virtual_generated(RelationGetDescr(relinfo->ri_RelationDesc), newtuple); + TupleDesc tupdesc = RelationGetDescr(relinfo->ri_RelationDesc); + /* + * set virtual generated columns value to null if them are not null. + * see ExecBRUpdateTriggers comments. also + */ + if (tupdesc->constr && tupdesc->constr->has_generated_virtual) + { + Datum replValues = 0; + bool replIsnull = true; + for (int j = 1; j <= tupdesc->natts; j++) + { + if ((TupleDescAttr(tupdesc, j-1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) + && !heap_attisnull(newtuple, j, tupdesc)) + newtuple = heap_modify_tuple_by_cols(newtuple, tupdesc, + 1, + &j, + &replValues, + &replIsnull); + } + } ExecForceStoreHeapTuple(newtuple, slot, false); @@ -3065,7 +3083,27 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, } else if (newtuple != oldtuple) { - check_modified_virtual_generated(RelationGetDescr(relinfo->ri_RelationDesc), newtuple); + TupleDesc tupdesc = RelationGetDescr(relinfo->ri_RelationDesc); + /* + * virtual generated columns have no storage, them will not insert to the + * table, so if any trigger set them to not-null value, set it to + * null again. this apply to ExecBRInsertTriggers also. + */ + if (tupdesc->constr && tupdesc->constr->has_generated_virtual) + { + Datum replValues = 0; + bool replIsnull = true; + for (int j = 1; j <= tupdesc->natts; j++) + { + if ((TupleDescAttr(tupdesc, j-1)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) + && !heap_attisnull(newtuple, j, tupdesc)) + newtuple = heap_modify_tuple_by_cols(newtuple, tupdesc, + 1, + &j, + &replValues, + &replIsnull); + } + } ExecForceStoreHeapTuple(newtuple, newslot, false); @@ -6609,36 +6647,3 @@ pg_trigger_depth(PG_FUNCTION_ARGS) { PG_RETURN_INT32(MyTriggerDepth); } - -/* - * Check whether a trigger modified a virtual generated column and error if - * so. - * - * We need to check this so that we don't end up storing a non-null value in a - * virtual generated column. - * - * We don't need to check for stored generated columns, since those will be - * overwritten later anyway. - * - * Alternatively, we could fix erroneous tuples here and be silent about it. - * This would yield the same user-facing behavior for virtual and stored - * generated columns. But it seems more complicated and not very useful in - * practice. - */ -static void -check_modified_virtual_generated(TupleDesc tupdesc, HeapTuple tuple) -{ - if (!(tupdesc->constr && tupdesc->constr->has_generated_virtual)) - return; - - for (int i = 0; i < tupdesc->natts; i++) - { - if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) - { - if (!heap_attisnull(tuple, i + 1, tupdesc)) - ereport(ERROR, - (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED), - errmsg("trigger modified virtual generated column value"))); - } - } -} diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index e31206e7f4..9bb7d6621a 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -992,12 +992,14 @@ plpgsql_exec_trigger(PLpgSQL_function *func, * still contains the old value.) Alternatively, we could construct a * whole new row structure without the generated columns, but this way * seems more efficient and potentially less confusing. + * we also make virtual generated columns be null in the NEW row. */ if (tupdesc->constr && tupdesc->constr->has_generated_stored && TRIGGER_FIRED_BEFORE(trigdata->tg_event)) { for (int i = 0; i < tupdesc->natts; i++) - if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_STORED) + if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_STORED || + TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) expanded_record_set_field_internal(rec_new->erh, i + 1, (Datum) 0, diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out index 042e184ea0..7dbda2b67d 100644 --- a/src/test/regress/expected/generated_virtual.out +++ b/src/test/regress/expected/generated_virtual.out @@ -1333,24 +1333,27 @@ BEGIN RETURN NEW; END; $$; -CREATE TRIGGER gtest12_01 BEFORE UPDATE ON gtest26 +CREATE TRIGGER gtest12_01 BEFORE UPDATE OR INSERT ON gtest26 FOR EACH ROW EXECUTE PROCEDURE gtest_trigger_func(); -CREATE TRIGGER gtest12_02 BEFORE UPDATE ON gtest26 +CREATE TRIGGER gtest12_02 BEFORE UPDATE OR INSERT ON gtest26 FOR EACH ROW EXECUTE PROCEDURE gtest_trigger_func4(); -CREATE TRIGGER gtest12_03 BEFORE UPDATE ON gtest26 +CREATE TRIGGER gtest12_03 BEFORE UPDATE OR INSERT ON gtest26 FOR EACH ROW EXECUTE PROCEDURE gtest_trigger_func(); INSERT INTO gtest26 (a) VALUES (1); -UPDATE gtest26 SET a = 11 WHERE a = 1; -INFO: gtest12_01: BEFORE: old = (1,) +INFO: gtest12_01: BEFORE: new = (1,) +INFO: gtest12_03: BEFORE: new = (10,) +UPDATE gtest26 SET a = 11 WHERE a = 10; +INFO: gtest12_01: BEFORE: old = (10,) INFO: gtest12_01: BEFORE: new = (11,) -ERROR: trigger modified virtual generated column value +INFO: gtest12_03: BEFORE: old = (10,) +INFO: gtest12_03: BEFORE: new = (10,) SELECT * FROM gtest26 ORDER BY a; - a | b ----+--- - 1 | 2 + a | b +----+---- + 10 | 20 (1 row) -- LIKE INCLUDING GENERATED and dropped column handling diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql index f97060ead4..2b0fc159af 100644 --- a/src/test/regress/sql/generated_virtual.sql +++ b/src/test/regress/sql/generated_virtual.sql @@ -695,20 +695,20 @@ BEGIN END; $$; -CREATE TRIGGER gtest12_01 BEFORE UPDATE ON gtest26 +CREATE TRIGGER gtest12_01 BEFORE UPDATE OR INSERT ON gtest26 FOR EACH ROW EXECUTE PROCEDURE gtest_trigger_func(); -CREATE TRIGGER gtest12_02 BEFORE UPDATE ON gtest26 +CREATE TRIGGER gtest12_02 BEFORE UPDATE OR INSERT ON gtest26 FOR EACH ROW EXECUTE PROCEDURE gtest_trigger_func4(); -CREATE TRIGGER gtest12_03 BEFORE UPDATE ON gtest26 +CREATE TRIGGER gtest12_03 BEFORE UPDATE OR INSERT ON gtest26 FOR EACH ROW EXECUTE PROCEDURE gtest_trigger_func(); INSERT INTO gtest26 (a) VALUES (1); -UPDATE gtest26 SET a = 11 WHERE a = 1; +UPDATE gtest26 SET a = 11 WHERE a = 10; SELECT * FROM gtest26 ORDER BY a; -- LIKE INCLUDING GENERATED and dropped column handling -- 2.34.1