public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Add one more planner strategy to JOIN with a partitioned relation.
7+ messages / 4 participants
[nested] [flat]
* [PATCH] Add one more planner strategy to JOIN with a partitioned relation.
@ 2020-08-21 05:38 Andrey Lepikhov <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Andrey Lepikhov @ 2020-08-21 05:38 UTC (permalink / raw)
Try to join inner relation with each partition of outer relation
and append results.
This strategy has potential benefits because it allows partitionwise
join with an unpartitioned relation or with a relation that is
partitioned by another schema.
---
src/backend/optimizer/path/allpaths.c | 9 +-
src/backend/optimizer/path/joinpath.c | 9 ++
src/backend/optimizer/path/joinrels.c | 132 +++++++++++++++++
src/backend/optimizer/plan/planner.c | 6 +-
src/backend/optimizer/util/appendinfo.c | 18 ++-
src/backend/optimizer/util/relnode.c | 14 +-
src/include/optimizer/paths.h | 10 +-
src/test/regress/expected/partition_join.out | 145 +++++++++++++++++++
src/test/regress/sql/partition_join.sql | 63 ++++++++
9 files changed, 385 insertions(+), 21 deletions(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 6da0dcd61c..4f110c5a2f 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1278,7 +1278,7 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
}
/* Add paths to the append relation. */
- add_paths_to_append_rel(root, rel, live_childrels);
+ add_paths_to_append_rel(root, rel, live_childrels, NIL);
}
@@ -1295,7 +1295,8 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
*/
void
add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
- List *live_childrels)
+ List *live_childrels,
+ List *original_partitioned_rels)
{
List *subpaths = NIL;
bool subpaths_valid = true;
@@ -1307,7 +1308,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
List *all_child_pathkeys = NIL;
List *all_child_outers = NIL;
ListCell *l;
- List *partitioned_rels = NIL;
+ List *partitioned_rels = original_partitioned_rels;
double partial_rows = -1;
/* If appropriate, consider parallel append */
@@ -3950,7 +3951,7 @@ generate_partitionwise_join_paths(PlannerInfo *root, RelOptInfo *rel)
}
/* Build additional paths for this rel from child-join paths. */
- add_paths_to_append_rel(root, rel, live_children);
+ add_paths_to_append_rel(root, rel, live_children, NIL);
list_free(live_children);
}
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index db54a6ba2e..36464e31aa 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -324,6 +324,15 @@ add_paths_to_joinrel(PlannerInfo *root,
if (set_join_pathlist_hook)
set_join_pathlist_hook(root, joinrel, outerrel, innerrel,
jointype, &extra);
+
+ /*
+ * 7. If outer relation is delivered from partition-tables, consider
+ * distributing inner relation to every partition-leaf prior to
+ * append these leafs.
+ */
+ try_asymmetric_partitionwise_join(root, joinrel,
+ outerrel, innerrel,
+ jointype, &extra);
}
/*
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 2d343cd293..4a7d0d0604 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -16,6 +16,7 @@
#include "miscadmin.h"
#include "optimizer/appendinfo.h"
+#include "optimizer/cost.h"
#include "optimizer/joininfo.h"
#include "optimizer/pathnode.h"
#include "optimizer/paths.h"
@@ -1551,6 +1552,137 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
}
}
+static List *
+extract_asymmetric_partitionwise_subjoin(PlannerInfo *root,
+ RelOptInfo *joinrel,
+ AppendPath *append_path,
+ RelOptInfo *inner_rel,
+ JoinType jointype,
+ JoinPathExtraData *extra)
+{
+ List *result = NIL;
+ ListCell *lc;
+
+ foreach (lc, append_path->subpaths)
+ {
+ Path *child_path = lfirst(lc);
+ RelOptInfo *child_rel = child_path->parent;
+ Relids child_join_relids;
+ RelOptInfo *child_join_rel;
+ SpecialJoinInfo *child_sjinfo;
+ List *child_restrictlist;
+ AppendRelInfo **appinfos;
+ int nappinfos;
+
+ child_join_relids = bms_union(child_rel->relids,
+ inner_rel->relids);
+ appinfos = find_appinfos_by_relids(root, child_join_relids,
+ &nappinfos);
+ child_sjinfo = build_child_join_sjinfo(root, extra->sjinfo,
+ child_rel->relids,
+ inner_rel->relids);
+ child_restrictlist = (List *)
+ adjust_appendrel_attrs(root, (Node *)extra->restrictlist,
+ nappinfos, appinfos);
+ pfree(appinfos);
+
+ child_join_rel = find_join_rel(root, child_join_relids);
+ if (!child_join_rel)
+ {
+ child_join_rel = build_child_join_rel(root,
+ child_rel,
+ inner_rel,
+ joinrel,
+ child_restrictlist,
+ child_sjinfo,
+ jointype);
+ if (!child_join_rel)
+ return NIL;
+ }
+
+ populate_joinrel_with_paths(root,
+ child_rel,
+ inner_rel,
+ child_join_rel,
+ child_sjinfo,
+ child_restrictlist);
+
+ /* Give up if asymmetric partition-wise join is not available */
+ if (child_join_rel->pathlist == NIL)
+ return NIL;
+
+ set_cheapest(child_join_rel);
+ result = lappend(result, child_join_rel);
+ }
+ return result;
+}
+
+void
+try_asymmetric_partitionwise_join(PlannerInfo *root,
+ RelOptInfo *joinrel,
+ RelOptInfo *outer_rel,
+ RelOptInfo *inner_rel,
+ JoinType jointype,
+ JoinPathExtraData *extra)
+{
+ ListCell *lc;
+
+ if (!enable_partitionwise_join)
+ return;
+
+ if (IS_OTHER_REL(outer_rel) || IS_OTHER_REL(inner_rel))
+ return;
+
+ if (jointype != JOIN_INNER && jointype != JOIN_LEFT)
+ return;
+
+ foreach (lc, outer_rel->pathlist)
+ {
+ AppendPath *append_path = lfirst(lc);
+
+ /*
+ * MEMO: We assume this pathlist keeps at least one AppendPath that
+ * represents partitioned table-scan, symmetric or asymmetric
+ * partition-wise join. It is not correct right now, however, a hook
+ * on add_path() to give additional decision for path removel allows
+ * to retain this kind of AppendPath, regardless of its cost.
+ */
+ if (IsA(append_path, AppendPath) &&
+ append_path->partitioned_rels != NIL)
+ {
+ List **join_rel_level_saved;
+ List *live_childrels = NIL;
+
+ join_rel_level_saved = root->join_rel_level;
+ PG_TRY();
+ {
+ /* temporary disables "dynamic programming" algorithm */
+ root->join_rel_level = NULL;
+
+ live_childrels =
+ extract_asymmetric_partitionwise_subjoin(root,
+ joinrel,
+ append_path,
+ inner_rel,
+ jointype,
+ extra);
+ }
+ PG_CATCH();
+ {
+ root->join_rel_level = join_rel_level_saved;
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+ root->join_rel_level = join_rel_level_saved;
+
+ if (live_childrels != NIL)
+ add_paths_to_append_rel(root, joinrel, live_childrels,
+ append_path->partitioned_rels);
+ break;
+ }
+ }
+}
+
/*
* Construct the SpecialJoinInfo for a child-join by translating
* SpecialJoinInfo for the join between parents. left_relids and right_relids
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index b40a112c25..863fb79f03 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -7536,7 +7536,7 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
}
/* Build new paths for this relation by appending child paths. */
- add_paths_to_append_rel(root, rel, live_children);
+ add_paths_to_append_rel(root, rel, live_children, NIL);
}
/*
@@ -7689,7 +7689,7 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
Assert(partially_grouped_live_children != NIL);
add_paths_to_append_rel(root, partially_grouped_rel,
- partially_grouped_live_children);
+ partially_grouped_live_children, NIL);
/*
* We need call set_cheapest, since the finalization step will use the
@@ -7704,7 +7704,7 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
{
Assert(grouped_live_children != NIL);
- add_paths_to_append_rel(root, grouped_rel, grouped_live_children);
+ add_paths_to_append_rel(root, grouped_rel, grouped_live_children, NIL);
}
}
diff --git a/src/backend/optimizer/util/appendinfo.c b/src/backend/optimizer/util/appendinfo.c
index d722063cf3..32230cf877 100644
--- a/src/backend/optimizer/util/appendinfo.c
+++ b/src/backend/optimizer/util/appendinfo.c
@@ -201,8 +201,9 @@ adjust_appendrel_attrs(PlannerInfo *root, Node *node, int nappinfos,
context.nappinfos = nappinfos;
context.appinfos = appinfos;
- /* If there's nothing to adjust, don't call this function. */
- Assert(nappinfos >= 1 && appinfos != NULL);
+ /* If there's nothing to adjust, just return a duplication */
+ if (nappinfos == 0)
+ return copyObject(node);
/*
* Must be prepared to start with a Query or a bare expression tree.
@@ -579,6 +580,7 @@ adjust_child_relids_multilevel(PlannerInfo *root, Relids relids,
AppendRelInfo **appinfos;
int nappinfos;
Relids parent_relids = NULL;
+ Relids normal_relids = NULL;
Relids result;
Relids tmp_result = NULL;
int cnt;
@@ -593,12 +595,17 @@ adjust_child_relids_multilevel(PlannerInfo *root, Relids relids,
appinfos = find_appinfos_by_relids(root, child_relids, &nappinfos);
/* Construct relids set for the immediate parent of the given child. */
+ normal_relids = bms_copy(child_relids);
for (cnt = 0; cnt < nappinfos; cnt++)
{
AppendRelInfo *appinfo = appinfos[cnt];
parent_relids = bms_add_member(parent_relids, appinfo->parent_relid);
+ normal_relids = bms_del_member(normal_relids, appinfo->child_relid);
}
+ parent_relids = bms_union(parent_relids, normal_relids);
+ if (normal_relids)
+ bms_free(normal_relids);
/* Recurse if immediate parent is not the top parent. */
if (!bms_equal(parent_relids, top_parent_relids))
@@ -728,11 +735,11 @@ AppendRelInfo **
find_appinfos_by_relids(PlannerInfo *root, Relids relids, int *nappinfos)
{
AppendRelInfo **appinfos;
+ int nrooms = bms_num_members(relids);
int cnt = 0;
int i;
- *nappinfos = bms_num_members(relids);
- appinfos = (AppendRelInfo **) palloc(sizeof(AppendRelInfo *) * *nappinfos);
+ appinfos = (AppendRelInfo **) palloc(sizeof(AppendRelInfo *) * nrooms);
i = -1;
while ((i = bms_next_member(relids, i)) >= 0)
@@ -740,9 +747,10 @@ find_appinfos_by_relids(PlannerInfo *root, Relids relids, int *nappinfos)
AppendRelInfo *appinfo = root->append_rel_array[i];
if (!appinfo)
- elog(ERROR, "child rel %d not found in append_rel_array", i);
+ continue;
appinfos[cnt++] = appinfo;
}
+ *nappinfos = cnt;
return appinfos;
}
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index a203e6f1ff..7a52b14fff 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -791,11 +791,8 @@ build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
AppendRelInfo **appinfos;
int nappinfos;
- /* Only joins between "other" relations land here. */
- Assert(IS_OTHER_REL(outer_rel) && IS_OTHER_REL(inner_rel));
-
- /* The parent joinrel should have consider_partitionwise_join set. */
- Assert(parent_joinrel->consider_partitionwise_join);
+ /* Either of relations must be "other" relation at least. */
+ Assert(IS_OTHER_REL(outer_rel) || IS_OTHER_REL(inner_rel));
joinrel->reloptkind = RELOPT_OTHER_JOINREL;
joinrel->relids = bms_union(outer_rel->relids, inner_rel->relids);
@@ -852,8 +849,11 @@ build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel,
joinrel->nullable_partexprs = NULL;
joinrel->partitioned_child_rels = NIL;
- joinrel->top_parent_relids = bms_union(outer_rel->top_parent_relids,
- inner_rel->top_parent_relids);
+ joinrel->top_parent_relids =
+ bms_union(IS_OTHER_REL(outer_rel) ?
+ outer_rel->top_parent_relids : outer_rel->relids,
+ IS_OTHER_REL(inner_rel) ?
+ inner_rel->top_parent_relids : inner_rel->relids);
/* Compute information relevant to foreign relations. */
set_foreign_rel_properties(joinrel, outer_rel, inner_rel);
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 10b6e81079..9949ff1d5d 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -108,7 +108,12 @@ extern bool have_join_order_restriction(PlannerInfo *root,
extern bool have_dangerous_phv(PlannerInfo *root,
Relids outer_relids, Relids inner_params);
extern void mark_dummy_rel(RelOptInfo *rel);
-
+extern void try_asymmetric_partitionwise_join(PlannerInfo *root,
+ RelOptInfo *joinrel,
+ RelOptInfo *outer_rel,
+ RelOptInfo *inner_rel,
+ JoinType jointype,
+ JoinPathExtraData *extra);
/*
* equivclass.c
* routines for managing EquivalenceClasses
@@ -239,6 +244,7 @@ extern PathKey *make_canonical_pathkey(PlannerInfo *root,
EquivalenceClass *eclass, Oid opfamily,
int strategy, bool nulls_first);
extern void add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
- List *live_childrels);
+ List *live_childrels,
+ List *original_partitioned_rels);
#endif /* PATHS_H */
diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.out
index 585e724375..01a0a17aac 100644
--- a/src/test/regress/expected/partition_join.out
+++ b/src/test/regress/expected/partition_join.out
@@ -2330,6 +2330,151 @@ SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_adv t1 INNER JOIN prt2_adv t2 ON (t1.a =
375 | 0375 | 375 | 0375
(8 rows)
+--
+-- For asymmetric partition-wise join
+--
+CREATE TABLE prt5 (hkey int, a int, b int) PARTITION BY HASH(hkey);
+CREATE TABLE prt5_p0 PARTITION OF prt5
+ FOR VALUES WITH (modulus 3, remainder 0);
+CREATE TABLE prt5_p1 PARTITION OF prt5
+ FOR VALUES WITH (modulus 3, remainder 1);
+CREATE TABLE prt5_p2 PARTITION OF prt5
+ FOR VALUES WITH (modulus 3, remainder 2);
+CREATE TABLE t5_1 (aid int, alabel text);
+CREATE TABLE t5_2 (bid int, blabel text);
+INSERT INTO prt5 (SELECT x, (1000.0 * random())::int,
+ (1000.0 * random())::int
+ FROM generate_series(1,1000000) x);
+INSERT INTO t5_1 (SELECT x, md5(x::text) FROM generate_series(-200, 800) x);
+INSERT INTO t5_2 (SELECT x, md5(x::text) FROM generate_series(-200, 800) x);
+VACUUM ANALYZE prt5;
+VACUUM ANALYZE t5_1;
+VACUUM ANALYZE t5_2;
+SET max_parallel_workers_per_gather = 0;
+EXPLAIN (COSTS OFF)
+SELECT *
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+ QUERY PLAN
+-------------------------------------------------------
+ Append
+ -> Hash Join
+ Hash Cond: (prt5_1.a = t5_1.aid)
+ -> Seq Scan on prt5_p0 prt5_1
+ -> Hash
+ -> Seq Scan on t5_1
+ Filter: (alabel ~~ '%abc%'::text)
+ -> Hash Join
+ Hash Cond: (prt5_2.a = t5_1.aid)
+ -> Seq Scan on prt5_p1 prt5_2
+ -> Hash
+ -> Seq Scan on t5_1
+ Filter: (alabel ~~ '%abc%'::text)
+ -> Hash Join
+ Hash Cond: (prt5_3.a = t5_1.aid)
+ -> Seq Scan on prt5_p2 prt5_3
+ -> Hash
+ -> Seq Scan on t5_1
+ Filter: (alabel ~~ '%abc%'::text)
+(19 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT *
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+ JOIN t5_2 ON b = bid AND blabel like '%cd%';
+ QUERY PLAN
+------------------------------------------------------------
+ Append
+ -> Hash Join
+ Hash Cond: (prt5_1.a = t5_1.aid)
+ -> Hash Join
+ Hash Cond: (prt5_1.b = t5_2.bid)
+ -> Seq Scan on prt5_p0 prt5_1
+ -> Hash
+ -> Seq Scan on t5_2
+ Filter: (blabel ~~ '%cd%'::text)
+ -> Hash
+ -> Seq Scan on t5_1
+ Filter: (alabel ~~ '%ab%'::text)
+ -> Hash Join
+ Hash Cond: (prt5_2.a = t5_1.aid)
+ -> Hash Join
+ Hash Cond: (prt5_2.b = t5_2.bid)
+ -> Seq Scan on prt5_p1 prt5_2
+ -> Hash
+ -> Seq Scan on t5_2
+ Filter: (blabel ~~ '%cd%'::text)
+ -> Hash
+ -> Seq Scan on t5_1
+ Filter: (alabel ~~ '%ab%'::text)
+ -> Hash Join
+ Hash Cond: (prt5_3.a = t5_1.aid)
+ -> Hash Join
+ Hash Cond: (prt5_3.b = t5_2.bid)
+ -> Seq Scan on prt5_p2 prt5_3
+ -> Hash
+ -> Seq Scan on t5_2
+ Filter: (blabel ~~ '%cd%'::text)
+ -> Hash
+ -> Seq Scan on t5_1
+ Filter: (alabel ~~ '%ab%'::text)
+(34 rows)
+
+-- unable to extract non-partitioned right relation
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt5 RIGHT JOIN t5_1 ON a = aid AND alabel like '%abc%';
+ QUERY PLAN
+-----------------------------------------------
+ Hash Right Join
+ Hash Cond: (prt5.a = t5_1.aid)
+ Join Filter: (t5_1.alabel ~~ '%abc%'::text)
+ -> Append
+ -> Seq Scan on prt5_p0 prt5_1
+ -> Seq Scan on prt5_p1 prt5_2
+ -> Seq Scan on prt5_p2 prt5_3
+ -> Hash
+ -> Seq Scan on t5_1
+(9 rows)
+
+-- left side can be extracted, but no cost benefit
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt5 LEFT JOIN t5_1 ON a = aid AND alabel like '%abc%';
+ QUERY PLAN
+-------------------------------------------------
+ Hash Left Join
+ Hash Cond: (prt5.a = t5_1.aid)
+ -> Append
+ -> Seq Scan on prt5_p0 prt5_1
+ -> Seq Scan on prt5_p1 prt5_2
+ -> Seq Scan on prt5_p2 prt5_3
+ -> Hash
+ -> Seq Scan on t5_1
+ Filter: (alabel ~~ '%abc%'::text)
+(9 rows)
+
+-- validation of the results with/without asymmetric partition-wise join
+SELECT * INTO pg_temp.result01a
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+SELECT * INTO pg_temp.result02a
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+ JOIN t5_2 ON b = bid AND blabel like '%cd%';
+SET enable_partitionwise_join = off;
+SELECT * INTO pg_temp.result01b
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+SELECT * INTO pg_temp.result02b
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+ JOIN t5_2 ON b = bid AND blabel like '%cd%';
+SELECT * FROM pg_temp.result01a EXCEPT SELECT * FROM pg_temp.result01b;
+ hkey | a | b | aid | alabel
+------+---+---+-----+--------
+(0 rows)
+
+SELECT * FROM pg_temp.result02a EXCEPT SELECT * FROM pg_temp.result02b;
+ hkey | a | b | aid | alabel | bid | blabel
+------+---+---+-----+--------+-----+--------
+(0 rows)
+
+RESET max_parallel_workers_per_gather;
+SET enable_partitionwise_join = true;
-- semi join
EXPLAIN (COSTS OFF)
SELECT t1.* FROM prt1_adv t1 WHERE EXISTS (SELECT 1 FROM prt2_adv t2 WHERE t1.a = t2.b) AND t1.b = 0 ORDER BY t1.a;
diff --git a/src/test/regress/sql/partition_join.sql b/src/test/regress/sql/partition_join.sql
index 73606c86e5..7b1c5cc30b 100644
--- a/src/test/regress/sql/partition_join.sql
+++ b/src/test/regress/sql/partition_join.sql
@@ -536,6 +536,69 @@ EXPLAIN (COSTS OFF)
SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_adv t1 INNER JOIN prt2_adv t2 ON (t1.a = t2.b) WHERE t1.b = 0 ORDER BY t1.a, t2.b;
SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_adv t1 INNER JOIN prt2_adv t2 ON (t1.a = t2.b) WHERE t1.b = 0 ORDER BY t1.a, t2.b;
+--
+-- For asymmetric partition-wise join
+--
+CREATE TABLE prt5 (hkey int, a int, b int) PARTITION BY HASH(hkey);
+CREATE TABLE prt5_p0 PARTITION OF prt5
+ FOR VALUES WITH (modulus 3, remainder 0);
+CREATE TABLE prt5_p1 PARTITION OF prt5
+ FOR VALUES WITH (modulus 3, remainder 1);
+CREATE TABLE prt5_p2 PARTITION OF prt5
+ FOR VALUES WITH (modulus 3, remainder 2);
+CREATE TABLE t5_1 (aid int, alabel text);
+CREATE TABLE t5_2 (bid int, blabel text);
+
+INSERT INTO prt5 (SELECT x, (1000.0 * random())::int,
+ (1000.0 * random())::int
+ FROM generate_series(1,1000000) x);
+INSERT INTO t5_1 (SELECT x, md5(x::text) FROM generate_series(-200, 800) x);
+INSERT INTO t5_2 (SELECT x, md5(x::text) FROM generate_series(-200, 800) x);
+
+VACUUM ANALYZE prt5;
+VACUUM ANALYZE t5_1;
+VACUUM ANALYZE t5_2;
+
+SET max_parallel_workers_per_gather = 0;
+EXPLAIN (COSTS OFF)
+SELECT *
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+EXPLAIN (COSTS OFF)
+SELECT *
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+ JOIN t5_2 ON b = bid AND blabel like '%cd%';
+
+-- unable to extract non-partitioned right relation
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt5 RIGHT JOIN t5_1 ON a = aid AND alabel like '%abc%';
+-- left side can be extracted, but no cost benefit
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt5 LEFT JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+-- validation of the results with/without asymmetric partition-wise join
+SELECT * INTO pg_temp.result01a
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+SELECT * INTO pg_temp.result02a
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+ JOIN t5_2 ON b = bid AND blabel like '%cd%';
+
+SET enable_partitionwise_join = off;
+
+SELECT * INTO pg_temp.result01b
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%abc%';
+
+SELECT * INTO pg_temp.result02b
+ FROM prt5 JOIN t5_1 ON a = aid AND alabel like '%ab%'
+ JOIN t5_2 ON b = bid AND blabel like '%cd%';
+
+SELECT * FROM pg_temp.result01a EXCEPT SELECT * FROM pg_temp.result01b;
+SELECT * FROM pg_temp.result02a EXCEPT SELECT * FROM pg_temp.result02b;
+
+RESET max_parallel_workers_per_gather;
+SET enable_partitionwise_join = true;
+
-- semi join
EXPLAIN (COSTS OFF)
SELECT t1.* FROM prt1_adv t1 WHERE EXISTS (SELECT 1 FROM prt2_adv t2 WHERE t1.a = t2.b) AND t1.b = 0 ORDER BY t1.a;
--
2.25.1
--------------9BF3329CEA51FF68F9089F65--
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [fixed] Trigger test
@ 2025-02-19 03:15 Dmitrii Bondar <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Dmitrii Bondar @ 2025-02-19 03:15 UTC (permalink / raw)
To: [email protected]
Just a rebase.
Attachments:
[text/x-diff] v4-0001-Triggers-test-fix.patch (11.9K, ../../[email protected]/2-v4-0001-Triggers-test-fix.patch)
download | inline diff:
From 76c4ca0f63091551b3f579c2c74345438e3d62d7 Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <[email protected]>
Date: Wed, 19 Feb 2025 09:37:48 +0700
Subject: [PATCH v4] Triggers test fix
---
contrib/spi/refint.c | 25 ++++++++++++-----
src/test/regress/expected/triggers.out | 39 +++++++++++++-------------
src/test/regress/sql/triggers.sql | 10 +++----
3 files changed, 42 insertions(+), 32 deletions(-)
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index e1aef7cd2a..fd4ba1c069 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -81,6 +81,10 @@ check_primary_key(PG_FUNCTION_ARGS)
/* internal error */
elog(ERROR, "check_primary_key: must be fired for row");
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ /* internal error */
+ elog(ERROR, "check_primary_key: must be fired by AFTER trigger");
+
/* If INSERTion then must check Tuple to being inserted */
if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
tuple = trigdata->tg_trigtuple;
@@ -264,7 +268,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
#ifdef DEBUG_QUERY
elog(DEBUG4, "check_foreign_key: Enter Function");
#endif
-
/*
* Some checks first...
*/
@@ -284,6 +287,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
/* internal error */
elog(ERROR, "check_foreign_key: cannot process INSERT events");
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ /* internal error */
+ elog(ERROR, "check_foreign_key: must be fired by AFTER trigger");
+
/* Have to check tg_trigtuple - tuple being deleted */
trigtuple = trigdata->tg_trigtuple;
@@ -335,10 +342,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
kvals = (Datum *) palloc(nkeys * sizeof(Datum));
/*
- * Construct ident string as TriggerName $ TriggeredRelationId and try to
- * find prepared execution plan(s).
+ * Construct ident string as TriggerName $ TriggeredRelationId $ OperationType
+ * and try to find prepared execution plan(s).
*/
- snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
+ snprintf(ident, sizeof(ident), "%s$%u$%c", trigger->tgname, rel->rd_id, is_update ? 'U' : 'D');
plan = find_plan(ident, &FPlans, &nFPlans);
/* if there is no plan(s) then allocate argtypes for preparation */
@@ -570,7 +577,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
relname = args[0];
- snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
plan = find_plan(ident, &FPlans, &nFPlans);
ret = SPI_execp(plan->splan[r], kvals, NULL, tcount);
/* we have no NULLs - so we pass ^^^^ here */
@@ -592,10 +598,15 @@ check_foreign_key(PG_FUNCTION_ARGS)
}
else
{
+ const char* operation;
+
+ if (action == 'c')
+ operation = is_update ? "updated" : "deleted";
+ else
+ operation = "set to null";
#ifdef REFINT_VERBOSE
elog(NOTICE, "%s: " UINT64_FORMAT " tuple(s) of %s are %s",
- trigger->tgname, SPI_processed, relname,
- (action == 'c') ? "deleted" : "set to null");
+ trigger->tgname, SPI_processed, relname, operation);
#endif
}
args += nkeys + 1; /* to the next relation */
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 247c67c32a..e6f585d974 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -46,12 +46,12 @@ create unique index pkeys_i on pkeys (pkey1, pkey2);
-- (fkey3) --> fkeys2 (pkey23)
--
create trigger check_fkeys_pkey_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function
check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2');
create trigger check_fkeys_pkey2_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23');
--
@@ -59,7 +59,7 @@ create trigger check_fkeys_pkey2_exist
-- (fkey21, fkey22) --> pkeys (pkey1, pkey2)
--
create trigger check_fkeys2_pkey_exist
- before insert or update on fkeys2
+ after insert or update on fkeys2
for each row
execute procedure
check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
@@ -74,7 +74,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL;
-- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22)
--
create trigger check_pkeys_fkey_cascade
- before delete or update on pkeys
+ after delete or update on pkeys
for each row
execute procedure
check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
@@ -85,7 +85,7 @@ create trigger check_pkeys_fkey_cascade
-- fkeys (fkey3)
--
create trigger check_fkeys2_fkey_restrict
- before delete or update on fkeys2
+ after delete or update on fkeys2
for each row
execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');
insert into fkeys2 values (10, '1', 1);
@@ -116,12 +116,11 @@ delete from pkeys where pkey1 = 40 and pkey2 = '4';
NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted
update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 50 and pkey2 = '5';
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
-ERROR: "check_fkeys2_fkey_restrict": tuple is referenced in "fkeys"
-CONTEXT: SQL statement "delete from fkeys2 where fkey21 = $1 and fkey22 = $2 "
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are updated
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are updated
update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted
+ERROR: duplicate key value violates unique constraint "pkeys_i"
+DETAIL: Key (pkey1, pkey2)=(7, 70) already exists.
SELECT trigger_name, event_manipulation, event_object_schema, event_object_table,
action_order, action_condition, action_orientation, action_timing,
action_reference_old_table, action_reference_new_table
@@ -130,16 +129,16 @@ SELECT trigger_name, event_manipulation, event_object_schema, event_object_table
ORDER BY trigger_name COLLATE "C", 2;
trigger_name | event_manipulation | event_object_schema | event_object_table | action_order | action_condition | action_orientation | action_timing | action_reference_old_table | action_reference_new_table
----------------------------+--------------------+---------------------+--------------------+--------------+------------------+--------------------+---------------+----------------------------+----------------------------
- check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | BEFORE | |
- check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | BEFORE | |
- check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | BEFORE | |
- check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | BEFORE | |
- check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | BEFORE | |
- check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | BEFORE | |
- check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | BEFORE | |
+ check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | AFTER | |
+ check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | AFTER | |
+ check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | AFTER | |
+ check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | AFTER | |
+ check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | AFTER | |
+ check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | AFTER | |
+ check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | AFTER | |
(10 rows)
DROP TABLE pkeys;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 659972f113..e5a491be7a 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -57,13 +57,13 @@ create unique index pkeys_i on pkeys (pkey1, pkey2);
-- (fkey3) --> fkeys2 (pkey23)
--
create trigger check_fkeys_pkey_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function
check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2');
create trigger check_fkeys_pkey2_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23');
@@ -72,7 +72,7 @@ create trigger check_fkeys_pkey2_exist
-- (fkey21, fkey22) --> pkeys (pkey1, pkey2)
--
create trigger check_fkeys2_pkey_exist
- before insert or update on fkeys2
+ after insert or update on fkeys2
for each row
execute procedure
check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
@@ -88,7 +88,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL;
-- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22)
--
create trigger check_pkeys_fkey_cascade
- before delete or update on pkeys
+ after delete or update on pkeys
for each row
execute procedure
check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
@@ -100,7 +100,7 @@ create trigger check_pkeys_fkey_cascade
-- fkeys (fkey3)
--
create trigger check_fkeys2_fkey_restrict
- before delete or update on fkeys2
+ after delete or update on fkeys2
for each row
execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');
--
2.34.1
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [fixed] Trigger test
@ 2025-03-25 20:48 Lilian Ontowhee <[email protected]>
parent: Dmitrii Bondar <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Lilian Ontowhee @ 2025-03-25 20:48 UTC (permalink / raw)
To: [email protected]; +Cc: Dmitrii Bondar <[email protected]>
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Hi Dmitrii,
Paul Jungwirth and I reviewed this patch, and here are our comments:
1. The patch applies and tests pass.
2. The patch fixes a bug in contrib/spi, which is not really a practical extension, but rather examples of how to use SPI. The contrib/spi directory actually has four extensions: refint, autoinc, insert_username, and moddatetime. The patch is for refint, which is a way you could implement foreign keys if it weren't built in to Postgres.
3. Consider updating documentation for doc/src/contrib-spi.sgml, or any file as appropriate, to reflect the changes.
4. Are there any cases where check_primary_key() and check_foreign_key() should be called using a BEFORE trigger? Will this change break backwards compatibility? Consider adding a test with a BEFORE trigger to ensure the error "must be fired by AFTER trigger" is raised.
Thank you!
The new status of this patch is: Waiting on Author
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [fixed] Trigger test
@ 2025-03-26 09:45 Dmitrii Bondar <[email protected]>
parent: Lilian Ontowhee <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Dmitrii Bondar @ 2025-03-26 09:45 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
Hi!
Thank you for the review!
> 3. Consider updating documentation for doc/src/contrib-spi.sgml, or any
> file as appropriate, to reflect the changes.
The changes have now been added to doc/src/contrib-spi.sgml. I also
added a consideration note about interactions with BEFORE triggers.
> 4. Are there any cases where check_primary_key() and
> check_foreign_key() should be called using a BEFORE trigger? Will this
> change break backwards compatibility? Consider adding a test with a
> BEFORE trigger to ensure the error "must be fired by AFTER trigger" is
> raised.
The usage within BEFORE triggers appears to be entirely incorrect. The
functions check_primary_key() and check_foreign_key() are intended for
use in creating constraint triggers, which according to the
documentation must be AFTER ROW triggers. Therefore, any cases using
BEFORE triggers are invalid.
I have updated the test so that it now raises the error "must be fired
by AFTER trigger."
Can you also help me with the patch status? What status should I move
the patch to?
Attachments:
[text/x-diff] v5-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patch (15.4K, ../../[email protected]/2-v5-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patch)
download | inline diff:
From f94391f6812d6a9bbdd2c43f5b24373c08cb08bb Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <[email protected]>
Date: Wed, 26 Mar 2025 16:39:01 +0700
Subject: [PATCH v5] Triggers test fix with the invalid cache in refint.c
---
contrib/spi/refint.c | 25 +++++++----
doc/src/sgml/contrib-spi.sgml | 12 +++++-
src/test/regress/expected/triggers.out | 57 +++++++++++++++++---------
src/test/regress/sql/triggers.sql | 30 +++++++++++---
4 files changed, 90 insertions(+), 34 deletions(-)
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index e1aef7cd2a3..fd4ba1c0698 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -81,6 +81,10 @@ check_primary_key(PG_FUNCTION_ARGS)
/* internal error */
elog(ERROR, "check_primary_key: must be fired for row");
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ /* internal error */
+ elog(ERROR, "check_primary_key: must be fired by AFTER trigger");
+
/* If INSERTion then must check Tuple to being inserted */
if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
tuple = trigdata->tg_trigtuple;
@@ -264,7 +268,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
#ifdef DEBUG_QUERY
elog(DEBUG4, "check_foreign_key: Enter Function");
#endif
-
/*
* Some checks first...
*/
@@ -284,6 +287,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
/* internal error */
elog(ERROR, "check_foreign_key: cannot process INSERT events");
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ /* internal error */
+ elog(ERROR, "check_foreign_key: must be fired by AFTER trigger");
+
/* Have to check tg_trigtuple - tuple being deleted */
trigtuple = trigdata->tg_trigtuple;
@@ -335,10 +342,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
kvals = (Datum *) palloc(nkeys * sizeof(Datum));
/*
- * Construct ident string as TriggerName $ TriggeredRelationId and try to
- * find prepared execution plan(s).
+ * Construct ident string as TriggerName $ TriggeredRelationId $ OperationType
+ * and try to find prepared execution plan(s).
*/
- snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
+ snprintf(ident, sizeof(ident), "%s$%u$%c", trigger->tgname, rel->rd_id, is_update ? 'U' : 'D');
plan = find_plan(ident, &FPlans, &nFPlans);
/* if there is no plan(s) then allocate argtypes for preparation */
@@ -570,7 +577,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
relname = args[0];
- snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
plan = find_plan(ident, &FPlans, &nFPlans);
ret = SPI_execp(plan->splan[r], kvals, NULL, tcount);
/* we have no NULLs - so we pass ^^^^ here */
@@ -592,10 +598,15 @@ check_foreign_key(PG_FUNCTION_ARGS)
}
else
{
+ const char* operation;
+
+ if (action == 'c')
+ operation = is_update ? "updated" : "deleted";
+ else
+ operation = "set to null";
#ifdef REFINT_VERBOSE
elog(NOTICE, "%s: " UINT64_FORMAT " tuple(s) of %s are %s",
- trigger->tgname, SPI_processed, relname,
- (action == 'c') ? "deleted" : "set to null");
+ trigger->tgname, SPI_processed, relname, operation);
#endif
}
args += nkeys + 1; /* to the next relation */
diff --git a/doc/src/sgml/contrib-spi.sgml b/doc/src/sgml/contrib-spi.sgml
index e7cae4e38dc..4fcc767a978 100644
--- a/doc/src/sgml/contrib-spi.sgml
+++ b/doc/src/sgml/contrib-spi.sgml
@@ -36,7 +36,7 @@
<para>
<function>check_primary_key()</function> checks the referencing table.
- To use, create a <literal>BEFORE INSERT OR UPDATE</literal> trigger using this
+ To use, create a <literal>AFTER INSERT OR UPDATE</literal> trigger using this
function on a table referencing another table. Specify as the trigger
arguments: the referencing table's column name(s) which form the foreign
key, the referenced table name, and the column names in the referenced table
@@ -46,7 +46,7 @@
<para>
<function>check_foreign_key()</function> checks the referenced table.
- To use, create a <literal>BEFORE DELETE OR UPDATE</literal> trigger using this
+ To use, create a <literal>AFTER DELETE OR UPDATE</literal> trigger using this
function on a table referenced by other table(s). Specify as the trigger
arguments: the number of referencing tables for which the function has to
perform checking, the action if a referencing key is found
@@ -63,6 +63,14 @@
<para>
There are examples in <filename>refint.example</filename>.
</para>
+
+ <para>
+ Note that if these triggers are executed from another <literal>BEFORE</literal>
+ trigger, they can fail unexpectedly. For example, if a user inserts row1 and then
+ the <literal>BEFORE</literal> trigger inserts row2 and calls a trigger with the
+ <function>check_foreign_key()</function>, the <function>check_foreign_key()</function>
+ function will not see row1 and will fail.
+ </para>
</sect2>
<sect2 id="contrib-spi-autoinc">
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 247c67c32ae..3b8bc558140 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -46,12 +46,12 @@ create unique index pkeys_i on pkeys (pkey1, pkey2);
-- (fkey3) --> fkeys2 (pkey23)
--
create trigger check_fkeys_pkey_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function
check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2');
create trigger check_fkeys_pkey2_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23');
--
@@ -59,7 +59,7 @@ create trigger check_fkeys_pkey2_exist
-- (fkey21, fkey22) --> pkeys (pkey1, pkey2)
--
create trigger check_fkeys2_pkey_exist
- before insert or update on fkeys2
+ after insert or update on fkeys2
for each row
execute procedure
check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
@@ -74,7 +74,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL;
-- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22)
--
create trigger check_pkeys_fkey_cascade
- before delete or update on pkeys
+ after delete or update on pkeys
for each row
execute procedure
check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
@@ -85,9 +85,27 @@ create trigger check_pkeys_fkey_cascade
-- fkeys (fkey3)
--
create trigger check_fkeys2_fkey_restrict
- before delete or update on fkeys2
+ after delete or update on fkeys2
for each row
execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');
+-- BEFORE triggers must raise an error
+create trigger check_fkeys2_pkey_exist_before
+ before insert or update on fkeys2
+ for each row
+ execute procedure
+ check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
+create trigger check_pkeys_fkey_cascade_before
+ before delete or update on pkeys
+ for each row
+ execute procedure
+ check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
+ 'fkeys', 'fkey1', 'fkey2', 'fkeys2', 'fkey21', 'fkey22');
+update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
+ERROR: check_foreign_key: must be fired by AFTER trigger
+insert into fkeys2 values (10, '1', 1);
+ERROR: check_primary_key: must be fired by AFTER trigger
+drop trigger check_fkeys2_pkey_exist_before on fkeys2;
+drop trigger check_pkeys_fkey_cascade_before on pkeys;
insert into fkeys2 values (10, '1', 1);
insert into fkeys2 values (30, '3', 2);
insert into fkeys2 values (40, '4', 5);
@@ -116,12 +134,11 @@ delete from pkeys where pkey1 = 40 and pkey2 = '4';
NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted
update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 50 and pkey2 = '5';
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
-ERROR: "check_fkeys2_fkey_restrict": tuple is referenced in "fkeys"
-CONTEXT: SQL statement "delete from fkeys2 where fkey21 = $1 and fkey22 = $2 "
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are updated
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are updated
update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted
+ERROR: duplicate key value violates unique constraint "pkeys_i"
+DETAIL: Key (pkey1, pkey2)=(7, 70) already exists.
SELECT trigger_name, event_manipulation, event_object_schema, event_object_table,
action_order, action_condition, action_orientation, action_timing,
action_reference_old_table, action_reference_new_table
@@ -130,16 +147,16 @@ SELECT trigger_name, event_manipulation, event_object_schema, event_object_table
ORDER BY trigger_name COLLATE "C", 2;
trigger_name | event_manipulation | event_object_schema | event_object_table | action_order | action_condition | action_orientation | action_timing | action_reference_old_table | action_reference_new_table
----------------------------+--------------------+---------------------+--------------------+--------------+------------------+--------------------+---------------+----------------------------+----------------------------
- check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | BEFORE | |
- check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | BEFORE | |
- check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | BEFORE | |
- check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | BEFORE | |
- check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | BEFORE | |
- check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | BEFORE | |
- check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | BEFORE | |
+ check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | AFTER | |
+ check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | AFTER | |
+ check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | AFTER | |
+ check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | AFTER | |
+ check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | AFTER | |
+ check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | AFTER | |
+ check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | AFTER | |
(10 rows)
DROP TABLE pkeys;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 659972f1135..7c9f6ec3b7e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -57,13 +57,13 @@ create unique index pkeys_i on pkeys (pkey1, pkey2);
-- (fkey3) --> fkeys2 (pkey23)
--
create trigger check_fkeys_pkey_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function
check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2');
create trigger check_fkeys_pkey2_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23');
@@ -72,7 +72,7 @@ create trigger check_fkeys_pkey2_exist
-- (fkey21, fkey22) --> pkeys (pkey1, pkey2)
--
create trigger check_fkeys2_pkey_exist
- before insert or update on fkeys2
+ after insert or update on fkeys2
for each row
execute procedure
check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
@@ -88,7 +88,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL;
-- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22)
--
create trigger check_pkeys_fkey_cascade
- before delete or update on pkeys
+ after delete or update on pkeys
for each row
execute procedure
check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
@@ -100,10 +100,30 @@ create trigger check_pkeys_fkey_cascade
-- fkeys (fkey3)
--
create trigger check_fkeys2_fkey_restrict
- before delete or update on fkeys2
+ after delete or update on fkeys2
for each row
execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');
+-- BEFORE triggers must raise an error
+create trigger check_fkeys2_pkey_exist_before
+ before insert or update on fkeys2
+ for each row
+ execute procedure
+ check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
+
+create trigger check_pkeys_fkey_cascade_before
+ before delete or update on pkeys
+ for each row
+ execute procedure
+ check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
+ 'fkeys', 'fkey1', 'fkey2', 'fkeys2', 'fkey21', 'fkey22');
+
+update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
+insert into fkeys2 values (10, '1', 1);
+
+drop trigger check_fkeys2_pkey_exist_before on fkeys2;
+drop trigger check_pkeys_fkey_cascade_before on pkeys;
+
insert into fkeys2 values (10, '1', 1);
insert into fkeys2 values (30, '3', 2);
insert into fkeys2 values (40, '4', 5);
--
2.34.1
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [fixed] Trigger test
@ 2025-03-26 22:46 Paul Jungwirth <[email protected]>
parent: Dmitrii Bondar <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Paul Jungwirth @ 2025-03-26 22:46 UTC (permalink / raw)
To: Dmitrii Bondar <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: Lilian <[email protected]>
Hi Dmitrii,
Thanks for the quick update!
On 3/26/25 02:45, Dmitrii Bondar wrote:
>> 3. Consider updating documentation for doc/src/contrib-spi.sgml, or any file as appropriate, to
>> reflect the changes.
>
> The changes have now been added to doc/src/contrib-spi.sgml. I also added a consideration note about
> interactions with BEFORE triggers.
This looks good. I have a couple small grammar suggestions. This:
+ To use, create a <literal>AFTER INSERT OR UPDATE</literal> trigger using this
should be:
+ To use, create an <literal>AFTER INSERT OR UPDATE</literal> trigger using this
and this:
+ To use, create a <literal>AFTER DELETE OR UPDATE</literal> trigger using this
should be this:
+ To use, create an <literal>AFTER DELETE OR UPDATE</literal> trigger using this
Also re this part of the patch:
@@ -592,10 +598,15 @@ check_foreign_key(PG_FUNCTION_ARGS)
}
else
{
+ const char* operation;
+
+ if (action == 'c')
+ operation = is_update ? "updated" : "deleted";
+ else
+ operation = "set to null";
#ifdef REFINT_VERBOSE
elog(NOTICE, "%s: " UINT64_FORMAT " tuple(s) of %s are %s",
- trigger->tgname, SPI_processed, relname,
- (action == 'c') ? "deleted" : "set to null");
+ trigger->tgname, SPI_processed, relname, operation);
#endif
}
args += nkeys + 1; /* to the next relation */
We can put all the new lines inside the #ifdef, can't we?
> Can you also help me with the patch status? What status should I move the patch to?
I think if you make those changes we should mark this as Ready for Committer.
Yours,
--
Paul ~{:-)
[email protected]
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [fixed] Trigger test
@ 2025-03-27 08:13 Dmitrii Bondar <[email protected]>
parent: Paul Jungwirth <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Dmitrii Bondar @ 2025-03-27 08:13 UTC (permalink / raw)
To: Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: Lilian <[email protected]>
Hi, Paul,
Thanks for the suggestions.
> This looks good. I have a couple small grammar suggestions. This:
I have replaced the incorrect articles with the correct ones.
> We can put all the new lines inside the #ifdef, can't we?
You're right. I have done that.
Best regards,
Dmitrii
Attachments:
[text/x-patch] v6-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patch (15.4K, ../../[email protected]/2-v6-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patch)
download | inline diff:
From f566662d52fcc6c2febb54dfb6f3aa5e128bcea1 Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <[email protected]>
Date: Thu, 27 Mar 2025 14:58:49 +0700
Subject: [PATCH v6] Triggers test fix with the invalid cache in refint.c
---
contrib/spi/refint.c | 26 ++++++++----
doc/src/sgml/contrib-spi.sgml | 12 +++++-
src/test/regress/expected/triggers.out | 57 +++++++++++++++++---------
src/test/regress/sql/triggers.sql | 30 +++++++++++---
4 files changed, 91 insertions(+), 34 deletions(-)
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index e1aef7cd2a3..b1e697eb935 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -81,6 +81,10 @@ check_primary_key(PG_FUNCTION_ARGS)
/* internal error */
elog(ERROR, "check_primary_key: must be fired for row");
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ /* internal error */
+ elog(ERROR, "check_primary_key: must be fired by AFTER trigger");
+
/* If INSERTion then must check Tuple to being inserted */
if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
tuple = trigdata->tg_trigtuple;
@@ -264,7 +268,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
#ifdef DEBUG_QUERY
elog(DEBUG4, "check_foreign_key: Enter Function");
#endif
-
/*
* Some checks first...
*/
@@ -284,6 +287,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
/* internal error */
elog(ERROR, "check_foreign_key: cannot process INSERT events");
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ /* internal error */
+ elog(ERROR, "check_foreign_key: must be fired by AFTER trigger");
+
/* Have to check tg_trigtuple - tuple being deleted */
trigtuple = trigdata->tg_trigtuple;
@@ -335,10 +342,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
kvals = (Datum *) palloc(nkeys * sizeof(Datum));
/*
- * Construct ident string as TriggerName $ TriggeredRelationId and try to
- * find prepared execution plan(s).
+ * Construct ident string as TriggerName $ TriggeredRelationId $ OperationType
+ * and try to find prepared execution plan(s).
*/
- snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
+ snprintf(ident, sizeof(ident), "%s$%u$%c", trigger->tgname, rel->rd_id, is_update ? 'U' : 'D');
plan = find_plan(ident, &FPlans, &nFPlans);
/* if there is no plan(s) then allocate argtypes for preparation */
@@ -570,7 +577,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
relname = args[0];
- snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
plan = find_plan(ident, &FPlans, &nFPlans);
ret = SPI_execp(plan->splan[r], kvals, NULL, tcount);
/* we have no NULLs - so we pass ^^^^ here */
@@ -593,9 +599,15 @@ check_foreign_key(PG_FUNCTION_ARGS)
else
{
#ifdef REFINT_VERBOSE
+ const char* operation;
+
+ if (action == 'c')
+ operation = is_update ? "updated" : "deleted";
+ else
+ operation = "set to null";
+
elog(NOTICE, "%s: " UINT64_FORMAT " tuple(s) of %s are %s",
- trigger->tgname, SPI_processed, relname,
- (action == 'c') ? "deleted" : "set to null");
+ trigger->tgname, SPI_processed, relname, operation);
#endif
}
args += nkeys + 1; /* to the next relation */
diff --git a/doc/src/sgml/contrib-spi.sgml b/doc/src/sgml/contrib-spi.sgml
index e7cae4e38dc..3a838199484 100644
--- a/doc/src/sgml/contrib-spi.sgml
+++ b/doc/src/sgml/contrib-spi.sgml
@@ -36,7 +36,7 @@
<para>
<function>check_primary_key()</function> checks the referencing table.
- To use, create a <literal>BEFORE INSERT OR UPDATE</literal> trigger using this
+ To use, create an <literal>AFTER INSERT OR UPDATE</literal> trigger using this
function on a table referencing another table. Specify as the trigger
arguments: the referencing table's column name(s) which form the foreign
key, the referenced table name, and the column names in the referenced table
@@ -46,7 +46,7 @@
<para>
<function>check_foreign_key()</function> checks the referenced table.
- To use, create a <literal>BEFORE DELETE OR UPDATE</literal> trigger using this
+ To use, create an <literal>AFTER DELETE OR UPDATE</literal> trigger using this
function on a table referenced by other table(s). Specify as the trigger
arguments: the number of referencing tables for which the function has to
perform checking, the action if a referencing key is found
@@ -63,6 +63,14 @@
<para>
There are examples in <filename>refint.example</filename>.
</para>
+
+ <para>
+ Note that if these triggers are executed from another <literal>BEFORE</literal>
+ trigger, they can fail unexpectedly. For example, if a user inserts row1 and then
+ the <literal>BEFORE</literal> trigger inserts row2 and calls a trigger with the
+ <function>check_foreign_key()</function>, the <function>check_foreign_key()</function>
+ function will not see row1 and will fail.
+ </para>
</sect2>
<sect2 id="contrib-spi-autoinc">
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 247c67c32ae..3b8bc558140 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -46,12 +46,12 @@ create unique index pkeys_i on pkeys (pkey1, pkey2);
-- (fkey3) --> fkeys2 (pkey23)
--
create trigger check_fkeys_pkey_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function
check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2');
create trigger check_fkeys_pkey2_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23');
--
@@ -59,7 +59,7 @@ create trigger check_fkeys_pkey2_exist
-- (fkey21, fkey22) --> pkeys (pkey1, pkey2)
--
create trigger check_fkeys2_pkey_exist
- before insert or update on fkeys2
+ after insert or update on fkeys2
for each row
execute procedure
check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
@@ -74,7 +74,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL;
-- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22)
--
create trigger check_pkeys_fkey_cascade
- before delete or update on pkeys
+ after delete or update on pkeys
for each row
execute procedure
check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
@@ -85,9 +85,27 @@ create trigger check_pkeys_fkey_cascade
-- fkeys (fkey3)
--
create trigger check_fkeys2_fkey_restrict
- before delete or update on fkeys2
+ after delete or update on fkeys2
for each row
execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');
+-- BEFORE triggers must raise an error
+create trigger check_fkeys2_pkey_exist_before
+ before insert or update on fkeys2
+ for each row
+ execute procedure
+ check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
+create trigger check_pkeys_fkey_cascade_before
+ before delete or update on pkeys
+ for each row
+ execute procedure
+ check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
+ 'fkeys', 'fkey1', 'fkey2', 'fkeys2', 'fkey21', 'fkey22');
+update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
+ERROR: check_foreign_key: must be fired by AFTER trigger
+insert into fkeys2 values (10, '1', 1);
+ERROR: check_primary_key: must be fired by AFTER trigger
+drop trigger check_fkeys2_pkey_exist_before on fkeys2;
+drop trigger check_pkeys_fkey_cascade_before on pkeys;
insert into fkeys2 values (10, '1', 1);
insert into fkeys2 values (30, '3', 2);
insert into fkeys2 values (40, '4', 5);
@@ -116,12 +134,11 @@ delete from pkeys where pkey1 = 40 and pkey2 = '4';
NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted
update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 50 and pkey2 = '5';
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
-ERROR: "check_fkeys2_fkey_restrict": tuple is referenced in "fkeys"
-CONTEXT: SQL statement "delete from fkeys2 where fkey21 = $1 and fkey22 = $2 "
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are updated
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are updated
update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted
+ERROR: duplicate key value violates unique constraint "pkeys_i"
+DETAIL: Key (pkey1, pkey2)=(7, 70) already exists.
SELECT trigger_name, event_manipulation, event_object_schema, event_object_table,
action_order, action_condition, action_orientation, action_timing,
action_reference_old_table, action_reference_new_table
@@ -130,16 +147,16 @@ SELECT trigger_name, event_manipulation, event_object_schema, event_object_table
ORDER BY trigger_name COLLATE "C", 2;
trigger_name | event_manipulation | event_object_schema | event_object_table | action_order | action_condition | action_orientation | action_timing | action_reference_old_table | action_reference_new_table
----------------------------+--------------------+---------------------+--------------------+--------------+------------------+--------------------+---------------+----------------------------+----------------------------
- check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | BEFORE | |
- check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | BEFORE | |
- check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | BEFORE | |
- check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | BEFORE | |
- check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | BEFORE | |
- check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | BEFORE | |
- check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | BEFORE | |
+ check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | AFTER | |
+ check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | AFTER | |
+ check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | AFTER | |
+ check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | AFTER | |
+ check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | AFTER | |
+ check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | AFTER | |
+ check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | AFTER | |
(10 rows)
DROP TABLE pkeys;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 659972f1135..7c9f6ec3b7e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -57,13 +57,13 @@ create unique index pkeys_i on pkeys (pkey1, pkey2);
-- (fkey3) --> fkeys2 (pkey23)
--
create trigger check_fkeys_pkey_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function
check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2');
create trigger check_fkeys_pkey2_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23');
@@ -72,7 +72,7 @@ create trigger check_fkeys_pkey2_exist
-- (fkey21, fkey22) --> pkeys (pkey1, pkey2)
--
create trigger check_fkeys2_pkey_exist
- before insert or update on fkeys2
+ after insert or update on fkeys2
for each row
execute procedure
check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
@@ -88,7 +88,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL;
-- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22)
--
create trigger check_pkeys_fkey_cascade
- before delete or update on pkeys
+ after delete or update on pkeys
for each row
execute procedure
check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
@@ -100,10 +100,30 @@ create trigger check_pkeys_fkey_cascade
-- fkeys (fkey3)
--
create trigger check_fkeys2_fkey_restrict
- before delete or update on fkeys2
+ after delete or update on fkeys2
for each row
execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');
+-- BEFORE triggers must raise an error
+create trigger check_fkeys2_pkey_exist_before
+ before insert or update on fkeys2
+ for each row
+ execute procedure
+ check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
+
+create trigger check_pkeys_fkey_cascade_before
+ before delete or update on pkeys
+ for each row
+ execute procedure
+ check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
+ 'fkeys', 'fkey1', 'fkey2', 'fkeys2', 'fkey21', 'fkey22');
+
+update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
+insert into fkeys2 values (10, '1', 1);
+
+drop trigger check_fkeys2_pkey_exist_before on fkeys2;
+drop trigger check_pkeys_fkey_cascade_before on pkeys;
+
insert into fkeys2 values (10, '1', 1);
insert into fkeys2 values (30, '3', 2);
insert into fkeys2 values (40, '4', 5);
--
2.34.1
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [fixed] Trigger test
@ 2025-04-04 05:45 Dmitrii Bondar <[email protected]>
parent: Dmitrii Bondar <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Dmitrii Bondar @ 2025-04-04 05:45 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Paul Jungwirth <[email protected]>; PostgreSQL Hackers <[email protected]>; Lilian <[email protected]>
On 04/04/2025 01:11, Tom Lane wrote:
> So that's a long laundry list and we haven't even dug hard.
> Is it worth it? If you feel like doing the legwork then
> I'm willing to support the project, but I really wonder if
> we shouldn't cut our losses and just remove the module.
>
> (I hesitate now to look at the rest of contrib/spi/ :-()
You wrote a note that I decided to omit. As I mentioned, the patch does
not even fix the cascade update problem—there are still broken
cases—because it seems impossible to address it in a gentle way (the
code was patched 20 years ago; it's truly legacy).
I considered removing it entirely, but that seemed too drastic a
solution (and, at the very least, I don't have enough expertise to make
that decision). If everything looks acceptable, I would prefer to cut
the module. The |check_primary_key| and |check_foreign| functions are
clearly unused, are buggy, and no one has reported any obvious
problems—so refint.c can be safely removed. Autoinc.c also looks
problematic.
There are some question. When should we remove the module? Should we
mark it as deprecated for now and remove it later? Should we handle it
in another thread? Should we apply this patch in that case?
Best regards,
Dmitrii
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2025-04-04 05:45 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21 05:38 [PATCH] Add one more planner strategy to JOIN with a partitioned relation. Andrey Lepikhov <[email protected]>
2025-02-19 03:15 Re: [fixed] Trigger test Dmitrii Bondar <[email protected]>
2025-03-25 20:48 ` Re: [fixed] Trigger test Lilian Ontowhee <[email protected]>
2025-03-26 09:45 ` Re: [fixed] Trigger test Dmitrii Bondar <[email protected]>
2025-03-26 22:46 ` Re: [fixed] Trigger test Paul Jungwirth <[email protected]>
2025-03-27 08:13 ` Re: [fixed] Trigger test Dmitrii Bondar <[email protected]>
2025-04-04 05:45 ` Re: [fixed] Trigger test Dmitrii Bondar <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox