Avoid CommandCounterIncrement in RI trigger when INSERT INTO referencing table
Hi hackers,
Currently, postgres increments command id in ri trigger every time when inserting into a referencing table(fk relation).
RI_FKey_check-> ri_PerformCheck->SPI_execute_snapshot-> _SPI_execute_plan-> CommandCounterIncrement
It can be a block for supporting "parallel insert into a referencing table", because we do not allow increment command id in parallel mode.
So, I was wondering if we can avoid incrementing command id in some cases when executing INSERT.
As far as I can see, it’s only necessary to increment command id when the INSERT command modified the referenced table.
And INSERT command only have one target table, the modification on other tables can happen in the following cases.
1) has modifyingcte which modifies the referenced table
2) has modifying function which modifies the referenced table.
(If I missed something please let me know)
Since the above two cases are not supported in parallel mode(parallel unsafe).
IMO, It seems it’s not necessary to increment command id in parallel mode,
we can just skip commandCounterIncrement when in parallel mode.
With this change, we can smoothly support "parallel insert into referencing table" which is desirable.
Part of what I plan to change is as the following:
-----------
RI_FKey_check_ins(PG_FUNCTION_ARGS)
{
+ bool needCCI = true;
+
/* Check that this is a valid trigger call on the right time and event. */
ri_CheckTrigger(fcinfo, "RI_FKey_check_ins", RI_TRIGTYPE_INSERT);
+ /*
+ * We do not need to increment the command counter
+ * in parallel mode, because any other modifications
+ * other than the insert event itself are parallel unsafe.
+ * So, there is no chance to modify the pk relation.
+ */
+ if (IsInParallelMode())
+ needCCI = false;
+
/* Share code with UPDATE case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, needCCI);
...
-----------Thoughts ?
Best regards,
houzj
Excuse me for asking probably stupid questions...
From: houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com>
As far as I can see, it’s only necessary to increment command id when the
INSERT command modified the referenced table.
Why do we have to increment the command ID when the INSERT's target table is a referenced table?
And INSERT command only have one target table, the modification on other
tables can happen in the following cases.1) has modifyingcte which modifies the referenced table
2) has modifying function which modifies the referenced table.
(If I missed something please let me know)
Also, why do we need CCI in these cases? What kind of problem would happen if we don't do CCI?
Since the above two cases are not supported in parallel mode(parallel unsafe).
IMO, It seems it’s not necessary to increment command id in parallel mode, we
can just skip commandCounterIncrement when in parallel mode.+ /* + * We do not need to increment the command counter + * in parallel mode, because any other modifications + * other than the insert event itself are parallel unsafe. + * So, there is no chance to modify the pk relation. + */ + if (IsInParallelMode()) + needCCI = false;
I'm worried about having this dependency in RI check, because the planner may allow parallel INSERT in these cases in the future.
Regards
Takayuki Tsunakawa
Hi,
Why do we have to increment the command ID when the INSERT's target table
is a referenced table?
Please refer to the explanation below.
And INSERT command only have one target table, the modification on
other tables can happen in the following cases.1) has modifyingcte which modifies the referenced table
2) has modifying function which modifies the referenced table.
(If I missed something please let me know)Also, why do we need CCI in these cases? What kind of problem would
happen if we don't do CCI?
From the wiki[1]https://wiki.postgresql.org/wiki/Developer_FAQ#What_is_CommandCounterIncrement.28.29.3F, CCI is to let statements can not see the rows they modify.
Here is an example of the case 1):
(Note table referenced and referencing are both empty)
-----
postgres=# with cte as (insert into referenced values(1)) insert into referencing values(1);
-----
The INSERT here will first modify the referenced table(pk table) and then modify the referencing table.
When modifying the referencing table, it has to check if the tuple to be insert exists in referenced table.
At this moment, CCI can let the modification on referenced in WITH visible when check the existence of the tuple.
If we do not CCI here, postgres will report an constraint error because it can not find the tuple in referenced table.
What do you think?
Since the above two cases are not supported in parallel mode(parallel
unsafe).
IMO, It seems it’s not necessary to increment command id in parallel
mode, we can just skip commandCounterIncrement when in parallel mode.+ /* + * We do not need to increment the command counter + * in parallel mode, because any other modifications + * other than the insert event itself are parallel unsafe. + * So, there is no chance to modify the pk relation. + */ + if (IsInParallelMode()) + needCCI = false;
I'm worried about having this dependency in RI check, because the planner may allow parallel INSERT in these cases in the future.
If we support parallel insert that can have other modifications in the future,
I think we will also be able to share or increment command ID in parallel wokers in that time.
And it seems we can remove this dependency in that time.
How about add some more comments about this to remind future developer.
/*
* If extra parallel modification is support in the future, this dependency should be removed.
*/
[1]: https://wiki.postgresql.org/wiki/Developer_FAQ#What_is_CommandCounterIncrement.28.29.3F
Best regards,
houzj
From the wiki[1], CCI is to let statements can not see the rows they modify.
Sorry, a typo here "not".
I meant CCI is to let statements can see the rows they modify.
Best regards,
houzj
From: Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com>
From the wiki[1], CCI is to let statements can not see the rows they modify.
Here is an example of the case 1):
(Note table referenced and referencing are both empty)
-----
postgres=# with cte as (insert into referenced values(1)) insert into
referencing values(1);
-----
The INSERT here will first modify the referenced table(pk table) and then
modify the referencing table.
When modifying the referencing table, it has to check if the tuple to be insert
exists in referenced table.
At this moment, CCI can let the modification on referenced in WITH visible
when check the existence of the tuple.
If we do not CCI here, postgres will report an constraint error because it can not
find the tuple in referenced table.
Ah, got it. Thank you. I'd regret if Postgres has to give up parallel execution because of SQL statements that don't seem to be used like the above.
I'm worried about having this dependency in RI check, because the planner
may allow parallel INSERT in these cases in the future.
If we support parallel insert that can have other modifications in the future, I
think we will also be able to share or increment command ID in parallel wokers
in that time.
And it seems we can remove this dependency in that time.
How about add some more comments about this to remind future developer.
/*
* If extra parallel modification is support in the future, this dependency should
be removed.
*/
Agreed.
I'm excited to see PostgreSQL's parallel DML work in wider use cases and satisfy users' expectations.
Regards
Takayuki Tsunakawa
I'm worried about having this dependency in RI check, because the
plannermay allow parallel INSERT in these cases in the future.
If we support parallel insert that can have other modifications in the
future, I think we will also be able to share or increment command ID
in parallel wokers in that time.
And it seems we can remove this dependency in that time.
How about add some more comments about this to remind futuredeveloper.
/*
* If extra parallel modification is support in the future, this
dependency should be removed.
*/Agreed.
I'm excited to see PostgreSQL's parallel DML work in wider use cases and
satisfy users' expectations.
Thanks !
Attaching the first version patch which avoid CCI in RI trigger when insert into referencing table.
Best regards,
houzj
Attachments:
0001-Avoid-CCI-in-RI-trigger-when-INSERT-fk-relation.patchapplication/octet-stream; name=0001-Avoid-CCI-in-RI-trigger-when-INSERT-fk-relation.patchDownload
From 7ca148b2567d5c7d6d281cef839cc3caa43e502b Mon Sep 17 00:00:00 2001
From: houzj <houzj.fnst@cn.fujitsu.com>
Date: Thu, 4 Mar 2021 14:13:23 +0800
Subject: [PATCH] Avoid CCI in RI trigger when INSERT fk relation
Currently, postgres increments command id in ri trigger every time
when inserting into a referencing table(fk relation).
It can be a block for supporting "parallel insert into a referencing table",
because we do not allow increment command id in parallel mode.
As far as I can see, it�fs only necessary to increment command id
when the INSERT command modified the referenced table.
And INSERT command only have one target table, the modification on
other tables can happen in the following cases.
* has modifyingcte which modifies the referenced table
* has modifying function which modifies the referenced table.
Since the above two cases are not supported in parallel mode(parallel unsafe).
IMO, It seems it�fs not necessary to increment command id in parallel mode,
we can just skip commandCounterIncrement when in parallel mode.
---
src/backend/utils/adt/ri_triggers.c | 43 ++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 6e3a410..f257227 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -213,7 +213,7 @@ static bool ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
TupleTableSlot *oldslot, TupleTableSlot *newslot,
- bool detectNewRows, int expect_OK);
+ bool detectNewRows, int expect_OK, bool modifypk);
static void ri_ExtractValues(Relation rel, TupleTableSlot *slot,
const RI_ConstraintInfo *riinfo, bool rel_is_pk,
Datum *vals, char *nulls);
@@ -229,7 +229,7 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo,
* Check foreign key existence (combined for INSERT and UPDATE).
*/
static Datum
-RI_FKey_check(TriggerData *trigdata)
+RI_FKey_check(TriggerData *trigdata, bool modifypk)
{
const RI_ConstraintInfo *riinfo;
Relation fk_rel;
@@ -393,7 +393,8 @@ RI_FKey_check(TriggerData *trigdata)
fk_rel, pk_rel,
NULL, newslot,
false,
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ modifypk);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -412,11 +413,24 @@ RI_FKey_check(TriggerData *trigdata)
Datum
RI_FKey_check_ins(PG_FUNCTION_ARGS)
{
+ bool modifypk = true;
+
/* Check that this is a valid trigger call on the right time and event. */
ri_CheckTrigger(fcinfo, "RI_FKey_check_ins", RI_TRIGTYPE_INSERT);
+ /*
+ * In parallel mode, any other modifications other
+ * than the insert event itself are parallel unsafe.
+ * So, there is no chance to modify the pk relation.
+ *
+ * Once we support extra modifications in parallel mode,
+ * this dependency is no longer needed.
+ */
+ if (IsInParallelMode())
+ modifypk = false;
+
/* Share code with UPDATE case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, modifypk);
}
@@ -432,7 +446,7 @@ RI_FKey_check_upd(PG_FUNCTION_ARGS)
ri_CheckTrigger(fcinfo, "RI_FKey_check_upd", RI_TRIGTYPE_UPDATE);
/* Share code with INSERT case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, true);
}
@@ -520,7 +534,8 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
fk_rel, pk_rel,
oldslot, NULL,
true, /* treat like update */
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -712,7 +727,8 @@ ri_restrict(TriggerData *trigdata, bool is_no_action)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -818,7 +834,8 @@ RI_FKey_cascade_del(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_DELETE);
+ SPI_OK_DELETE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -939,7 +956,8 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
oldslot, newslot,
true, /* must detect new rows */
- SPI_OK_UPDATE);
+ SPI_OK_UPDATE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -1118,7 +1136,8 @@ ri_set(TriggerData *trigdata, bool is_set_null)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_UPDATE);
+ SPI_OK_UPDATE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -2179,7 +2198,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
TupleTableSlot *oldslot, TupleTableSlot *newslot,
- bool detectNewRows, int expect_OK)
+ bool detectNewRows, int expect_OK, bool modifypk)
{
Relation query_rel,
source_rel;
@@ -2277,7 +2296,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
spi_result = SPI_execute_snapshot(qplan,
vals, nulls,
test_snapshot, crosscheck_snapshot,
- false, false, limit);
+ !modifypk, false, limit);
/* Restore UID and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
--
2.7.2.windows.1
Attaching the first version patch which avoid CCI in RI trigger when insert into
referencing table.
After some more on how to support parallel insert into fk relation.
It seems we do not have a cheap way to implement this feature.
Please see the explanation below:
In RI_FKey_check, Currently, postgres execute "select xx for key share" to check that foreign key exists in PK table.
However "select for update/share" is considered as parallel unsafe. It may be dangerous to do this in parallel mode, we may want to change this.
And also, "select for update/share" is considered as "not read only" which will force readonly = false in _SPI_execute_plan.
So, it seems we have to completely change the implementation of RI_FKey_check.
At the same time, " simplifying foreign key/RI checks " thread is trying to replace "select xx for key share" with index_beginscan()+table_tuple_lock() (I think it’s parallel safe).
May be we can try to support parallel insert fk relation after " simplifying foreign key/RI checks " patch applied ?
Best regards,
houzj
From: Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com>
After some more on how to support parallel insert into fk relation.
It seems we do not have a cheap way to implement this feature.In RI_FKey_check, Currently, postgres execute "select xx for key share" to
check that foreign key exists in PK table.
However "select for update/share" is considered as parallel unsafe. It may be
dangerous to do this in parallel mode, we may want to change this.
Hmm, I guess the parallel leader and workers can execute SELECT FOR KEY SHARE, if the parallelism infrastructure allows execution of SPI calls. The lock manager supports tuple locking in parallel leader and workers by the group locking. Also, the tuple locking doesn't require combo Cid, which is necessary for parallel UPDATE and DELETE.
Perhaps the reason why SELECT FOR is treated as parallel-unsafe is that tuple locking modifies data pages to store lock information in the tuple header. But now, page modification is possible in parallel processes, so I think we can consider SELECT FOR as parallel-safe. (I may be too optimistic.)
And also, "select for update/share" is considered as "not read only" which will
force readonly = false in _SPI_execute_plan.
read_only is used to do CCI. Can we arrange to skip CCI?
At the same time, " simplifying foreign key/RI checks " thread is trying to
replace "select xx for key share" with index_beginscan()+table_tuple_lock() (I
think it’s parallel safe).
May be we can try to support parallel insert fk relation after " simplifying foreign
key/RI checks " patch applied ?
Why do you think it's parallel safe?
Can you try running parallel INSERT SELECT on the target table with FK and see if any problem happens?
If some problem occurs due to the tuple locking, I think we can work around it by avoiding tuple locking. That is, we make parallel INSERT SELECT lock the parent tables in exclusive mode so that the check tuples won't be deleted. Some people may not like this, but it's worth considering because parallel INSERT SELECT would not have to be run concurrently with short OLTP transactions. Anyway, tuple locking partly disturbs parallel INSERT speedup because it modifies pages in the parent tables and emits WAL.
Surprisingly, Oracle doesn't support parallel INSERT SELECT on a table with FK as follows. SQL Server doesn't mention anything, so I guess it's supported. This is a good chance for PostgreSQL to exceed Oracle.
Table 8-1 Referential Integrity Restrictions
DML Statement Issued on Parent Issued on Child Self-Referential
INSERT (Not applicable) Not parallelized Not parallelized
Regards
Takayuki Tsunakawa
After some more on how to support parallel insert into fk relation.
It seems we do not have a cheap way to implement this feature.In RI_FKey_check, Currently, postgres execute "select xx for key
share" to check that foreign key exists in PK table.
However "select for update/share" is considered as parallel unsafe. It
may be dangerous to do this in parallel mode, we may want to change this.Hmm, I guess the parallel leader and workers can execute SELECT FOR KEY
SHARE, if the parallelism infrastructure allows execution of SPI calls. The lock
manager supports tuple locking in parallel leader and workers by the group
locking. Also, the tuple locking doesn't require combo Cid, which is necessary
for parallel UPDATE and DELETE.Perhaps the reason why SELECT FOR is treated as parallel-unsafe is that tuple
locking modifies data pages to store lock information in the tuple header. But
now, page modification is possible in parallel processes, so I think we can
consider SELECT FOR as parallel-safe. (I may be too optimistic.)
I think you are right.
After reading the original parallel-safety check's commit message , README.tuplock, and have some discussions with the author.
I think the reason why [SELECT FOR UPDATE/SHARE] is parallel unsafe is that [SELECT FOR] will call GetCurrentCommandId(true).
GetCurrentCommandId(true) was not supported in parallel worker but [SELECT FOR] need command ID to mark the change(lock info).
Fortunately, With greg's parallel insert patch, we can use command ID in parallel worker.
So, IMO, In parallel insert case, the RI check is parallel safe.
And also, "select for update/share" is considered as "not read only"
which will force readonly = false in _SPI_execute_plan.read_only is used to do CCI. Can we arrange to skip CCI?
Yes, we can.
Currently, I try to add one parameter(need CCI) to SPI_execute_snapshot and _SPI_execute_plan to control CCI.
I was still researching is there a more elegant way.
At the same time, " simplifying foreign key/RI checks " thread is
trying to replace "select xx for key share" with
index_beginscan()+table_tuple_lock() (I think it’s parallel safe).
May be we can try to support parallel insert fk relation after "
simplifying foreign key/RI checks " patch applied ?Why do you think it's parallel safe?
Can you try running parallel INSERT SELECT on the target table with FK and see
if any problem happens?
I have tested this in various cases:
All the test results looks good.
* test different worker lock on the same tuple.
* test different worker lock on different tuple.
* test no lock.
* test lock with concurrent update
* test constraint error.
Surprisingly, Oracle doesn't support parallel INSERT SELECT on a table with FK
as follows. SQL Server doesn't mention anything, so I guess it's supported.
This is a good chance for PostgreSQL to exceed Oracle.Table 8-1 Referential Integrity Restrictions
DML Statement Issued on Parent Issued on Child
Self-Referential
INSERT (Not applicable) Not parallelized Not parallelized
Ah, that's really good chance.
To support parallel insert into FK relation.
There are two scenarios need attention.
1) foreign key and primary key are on the same table(INSERT's target table).
(referenced and referencing are the same table)
2) referenced and referencing table are both partition of INSERT's target table.
(These cases are really rare for me)
In the two cases, the referenced table could be modified when INSERTing and CCI is necessary,
So, I think we should treat these cases as parallel restricted while doing safety check.
Attaching V1 patch that Implemented above feature and passed regression test.
Best regards,
houzj
Attachments:
v1_0002-WIP-safety-check.patchapplication/octet-stream; name=v1_0002-WIP-safety-check.patchDownload
From cbcda1def53d637ad94fde463f2cfe4d5c55d3ce Mon Sep 17 00:00:00 2001
From: houzj <houzj.fnst@cn.fujitsu.com>
Date: Tue, 9 Mar 2021 14:48:59 +0800
Subject: [PATCH] foreign key
Extent the parallel-safety check to treat the following cases parallel restricted:
1) foreign key and primary key are on the same table(INSERT's target table).
(referenced and referencing are the same table)
2) referenced and referencing table are both partition of INSERT's target table.
---
src/backend/optimizer/util/clauses.c | 77 +++++++++++++++++++++++++++++++++---
1 file changed, 72 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index f54268c..a1928d9 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -100,6 +100,7 @@ typedef struct
RangeTblEntry *target_rte; /* query's target relation if any */
CmdType command_type; /* query's command type */
PlannerGlobal *planner_global; /* global info for planner invocation */
+ List *pk_rels; /* OIDs of relations referenced by target relation */
} max_parallel_hazard_context;
static bool contain_agg_clause_walker(Node *node, void *context);
@@ -582,6 +583,7 @@ max_parallel_hazard(Query *parse, PlannerGlobal *glob)
rt_fetch(parse->resultRelation, parse->rtable) : NULL;
context.command_type = parse->commandType;
context.planner_global = glob;
+ context.pk_rels = NIL;
(void) max_parallel_hazard_walker((Node *) parse, &context);
return context.max_hazard;
@@ -872,7 +874,7 @@ static bool
target_rel_max_parallel_hazard(max_parallel_hazard_context *context)
{
bool max_hazard_found;
-
+ ListCell *lc;
Relation targetRel;
/*
@@ -885,6 +887,35 @@ target_rel_max_parallel_hazard(max_parallel_hazard_context *context)
context->command_type,
context);
+ /*
+ * Check if target relation is one of PK relations.
+ */
+ if (!max_hazard_found && context->max_hazard == PROPARALLEL_SAFE)
+ {
+ if (list_member_oid(context->pk_rels, context->target_rte->relid))
+ {
+ max_hazard_found = max_parallel_hazard_test(PROPARALLEL_RESTRICTED,
+ context);
+ }
+ }
+
+ /*
+ * check if any pk relation is partition of the target table.
+ */
+ if (context->max_hazard == PROPARALLEL_SAFE)
+ {
+ foreach(lc, context->pk_rels)
+ {
+ if (list_member_oid(context->planner_global->partitionOids,
+ lfirst_oid(lc)))
+ {
+ max_hazard_found = max_parallel_hazard_test(PROPARALLEL_RESTRICTED,
+ context);
+ break;
+ }
+ }
+ }
+
table_close(targetRel, NoLock);
return max_hazard_found;
@@ -974,15 +1005,51 @@ target_rel_trigger_max_parallel_hazard(TriggerDesc *trigdesc,
/*
* If the trigger type is RI_TRIGGER_FK, this indicates a FK exists in
- * the relation, and this would result in creation of new CommandIds
- * on insert/update and this isn't supported in a parallel
- * worker (but is safe in the parallel leader).
+ * the relation, this should result in creation of new CommandIds if
+ * PK relation is modified.
+ *
+ * However, creation of new CommandIds is not supported in a
+ * parallel worker(but is safe in the parallel leader). So, treat all
+ * scenarios that may modify PK relation as parallel-restricted.
*/
trigtype = RI_FKey_trigger_type(trigger->tgfoid);
if (trigtype == RI_TRIGGER_FK)
{
- if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
+ HeapTuple tup;
+ Form_pg_constraint conForm;
+ Oid constraintOid;
+
+ constraintOid = trigger->tgconstraint;
+
+ /*
+ * Fetch the pg_constraint row so we can fill in the entry.
+ */
+ tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraintOid));
+ if (!HeapTupleIsValid(tup)) /* should not happen */
+ elog(ERROR, "cache lookup failed for constraint %u",
+ constraintOid);
+
+ conForm = (Form_pg_constraint) GETSTRUCT(tup);
+ if (conForm->contype != CONSTRAINT_FOREIGN) /* should not happen */
+ elog(ERROR, "constraint %u is not a foreign key constraint",
+ constraintOid);
+
+ /*
+ * If FK relation and PK relation are not the same,
+ * they could be partitions of the same table.
+ * Remember Oids of PK relation for the later check.
+ */
+ if (conForm->confrelid != conForm->conrelid)
+ context->pk_rels = lappend_oid(context->pk_rels, conForm->confrelid);
+
+ /*
+ * If FK relation and PK relation are the same, the PK relation
+ * could be modfied.
+ */
+ else if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
return true;
+
+ ReleaseSysCache(tup);
}
}
--
2.7.2.windows.1
v1_0001-WIP-avoid-cci.patchapplication/octet-stream; name=v1_0001-WIP-avoid-cci.patchDownload
From a63756e4c4097db223186fb575f74a35318ebf41 Mon Sep 17 00:00:00 2001
From: houzj <houzj.fnst@cn.fujitsu.com>
Date: Thu, 11 Mar 2021 18:31:27 +0800
Subject: [PATCH] avoid cci
---
src/backend/executor/spi.c | 27 +++++++++++----------
src/backend/utils/adt/ri_triggers.c | 47 ++++++++++++++++++++++++++-----------
src/include/executor/spi.h | 3 ++-
3 files changed, 50 insertions(+), 27 deletions(-)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 00aa78e..3b84d8c 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -69,7 +69,8 @@ static int _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
bool read_only, bool no_snapshots,
bool fire_triggers, uint64 tcount,
DestReceiver *caller_dest,
- ResourceOwner plan_owner);
+ ResourceOwner plan_owner,
+ bool needCCI);
static ParamListInfo _SPI_convert_params(int nargs, Oid *argtypes,
Datum *Values, const char *Nulls);
@@ -525,7 +526,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, true);
_SPI_end_call(true);
return res;
@@ -569,7 +570,7 @@ SPI_execute_extended(const char *src,
InvalidSnapshot, InvalidSnapshot,
options->read_only, options->no_snapshots,
true, options->tcount,
- options->dest, options->owner);
+ options->dest, options->owner, true);
_SPI_end_call(true);
return res;
@@ -598,7 +599,7 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, true);
_SPI_end_call(true);
return res;
@@ -629,7 +630,7 @@ SPI_execute_plan_extended(SPIPlanPtr plan,
InvalidSnapshot, InvalidSnapshot,
options->read_only, options->no_snapshots,
true, options->tcount,
- options->dest, options->owner);
+ options->dest, options->owner, true);
_SPI_end_call(true);
return res;
@@ -653,7 +654,7 @@ SPI_execute_plan_with_paramlist(SPIPlanPtr plan, ParamListInfo params,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, true);
_SPI_end_call(true);
return res;
@@ -676,7 +677,8 @@ int
SPI_execute_snapshot(SPIPlanPtr plan,
Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
- bool read_only, bool fire_triggers, long tcount)
+ bool read_only, bool fire_triggers, long tcount,
+ bool needCCI)
{
int res;
@@ -696,7 +698,7 @@ SPI_execute_snapshot(SPIPlanPtr plan,
snapshot, crosscheck_snapshot,
read_only, false,
fire_triggers, tcount,
- NULL, NULL);
+ NULL, NULL, needCCI);
_SPI_end_call(true);
return res;
@@ -746,7 +748,7 @@ SPI_execute_with_args(const char *src,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, true);
_SPI_end_call(true);
return res;
@@ -2277,7 +2279,8 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
Snapshot snapshot, Snapshot crosscheck_snapshot,
bool read_only, bool no_snapshots,
bool fire_triggers, uint64 tcount,
- DestReceiver *caller_dest, ResourceOwner plan_owner)
+ DestReceiver *caller_dest, ResourceOwner plan_owner,
+ bool needCCI)
{
int my_res = 0;
uint64 my_processed = 0;
@@ -2464,7 +2467,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
* If not read-only mode, advance the command counter before each
* command and update the snapshot.
*/
- if (!read_only && !no_snapshots)
+ if (needCCI && !read_only && !no_snapshots)
{
CommandCounterIncrement();
UpdateActiveSnapshotCommandId();
@@ -2608,7 +2611,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
* command. This ensures that its effects are visible, in case it was
* DDL that would affect the next CachedPlanSource.
*/
- if (!read_only)
+ if (needCCI && !read_only)
CommandCounterIncrement();
}
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 6e3a410..468fb4f 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -213,7 +213,7 @@ static bool ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
TupleTableSlot *oldslot, TupleTableSlot *newslot,
- bool detectNewRows, int expect_OK);
+ bool detectNewRows, int expect_OK, bool modifypk);
static void ri_ExtractValues(Relation rel, TupleTableSlot *slot,
const RI_ConstraintInfo *riinfo, bool rel_is_pk,
Datum *vals, char *nulls);
@@ -229,7 +229,7 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo,
* Check foreign key existence (combined for INSERT and UPDATE).
*/
static Datum
-RI_FKey_check(TriggerData *trigdata)
+RI_FKey_check(TriggerData *trigdata, bool modifypk)
{
const RI_ConstraintInfo *riinfo;
Relation fk_rel;
@@ -393,7 +393,8 @@ RI_FKey_check(TriggerData *trigdata)
fk_rel, pk_rel,
NULL, newslot,
false,
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ modifypk);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -412,11 +413,24 @@ RI_FKey_check(TriggerData *trigdata)
Datum
RI_FKey_check_ins(PG_FUNCTION_ARGS)
{
+ bool modifypk = true;
+
/* Check that this is a valid trigger call on the right time and event. */
ri_CheckTrigger(fcinfo, "RI_FKey_check_ins", RI_TRIGTYPE_INSERT);
+ /*
+ * In parallel mode, any other modifications other
+ * than the insert event itself are parallel unsafe.
+ * So, there is no chance to modify the pk relation.
+ *
+ * Once we support extra modifications in parallel mode,
+ * this dependency is no longer needed.
+ */
+ if (IsInParallelMode())
+ modifypk = false;
+
/* Share code with UPDATE case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, modifypk);
}
@@ -432,7 +446,7 @@ RI_FKey_check_upd(PG_FUNCTION_ARGS)
ri_CheckTrigger(fcinfo, "RI_FKey_check_upd", RI_TRIGTYPE_UPDATE);
/* Share code with INSERT case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, true);
}
@@ -520,7 +534,8 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
fk_rel, pk_rel,
oldslot, NULL,
true, /* treat like update */
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -712,7 +727,8 @@ ri_restrict(TriggerData *trigdata, bool is_no_action)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -818,7 +834,8 @@ RI_FKey_cascade_del(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_DELETE);
+ SPI_OK_DELETE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -939,7 +956,8 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
oldslot, newslot,
true, /* must detect new rows */
- SPI_OK_UPDATE);
+ SPI_OK_UPDATE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -1118,7 +1136,8 @@ ri_set(TriggerData *trigdata, bool is_set_null)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_UPDATE);
+ SPI_OK_UPDATE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -1492,7 +1511,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
NULL, NULL,
GetLatestSnapshot(),
InvalidSnapshot,
- true, false, 1);
+ true, false, 1, true);
/* Check result */
if (spi_result != SPI_OK_SELECT)
@@ -1732,7 +1751,7 @@ RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
NULL, NULL,
GetLatestSnapshot(),
InvalidSnapshot,
- true, false, 1);
+ true, false, 1, true);
/* Check result */
if (spi_result != SPI_OK_SELECT)
@@ -2179,7 +2198,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
TupleTableSlot *oldslot, TupleTableSlot *newslot,
- bool detectNewRows, int expect_OK)
+ bool detectNewRows, int expect_OK, bool modifypk)
{
Relation query_rel,
source_rel;
@@ -2277,7 +2296,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
spi_result = SPI_execute_snapshot(qplan,
vals, nulls,
test_snapshot, crosscheck_snapshot,
- false, false, limit);
+ false, false, limit, modifypk);
/* Restore UID and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index 6455d10..3dc6b86 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -129,7 +129,8 @@ extern int SPI_execute_snapshot(SPIPlanPtr plan,
Datum *Values, const char *Nulls,
Snapshot snapshot,
Snapshot crosscheck_snapshot,
- bool read_only, bool fire_triggers, long tcount);
+ bool read_only, bool fire_triggers, long tcount,
+ bool needCCI);
extern int SPI_execute_with_args(const char *src,
int nargs, Oid *argtypes,
Datum *Values, const char *Nulls,
--
2.7.2.windows.1
To support parallel insert into FK relation.
There are two scenarios need attention.
1) foreign key and primary key are on the same table(INSERT's target table).
(referenced and referencing are the same table)
2) referenced and referencing table are both partition of INSERT's target table.
(These cases are really rare for me)In the two cases, the referenced table could be modified when INSERTing and
CCI is necessary, So, I think we should treat these cases as parallel restricted
while doing safety check.Attaching V1 patch that Implemented above feature and passed regression
test.
Attaching rebased patch based on HEAD.
Best regards,
houzj
Attachments:
v2-0002-extend-safery-check-for-fk-relation.patchapplication/octet-stream; name=v2-0002-extend-safery-check-for-fk-relation.patchDownload
From 6ed6237c1b9c06a6603e7c817da2f6308b24591d Mon Sep 17 00:00:00 2001
From: houzj <houzj.fnst@cn.fujitsu.com>
Date: Tue, 16 Mar 2021 15:28:10 +0800
Subject: [PATCH] extend safery check to support parallel insert into fk relation
Currently, We can not support parallel insert into fk relation in all cases.
For example:
When inserting into a table with foreign key, if the referenced could also be modified in
INSERT command, we will need to do CommandCounterIncrement to let the modification
on referenced table visible for the RI check which is not supported in parallel worker.
So, Extent the parallel-safety check to treat the following cases(could modify referenced table) parallel restricted:
1) foreign key and primary key are on the same table(INSERT's target table).
(referenced and referencing are the same table)
2) referenced and referencing table are both partition of INSERT's target table.
(Note: modifyingCTE and function with modifying statement could also modifiy the referenced table,
However, the current parallel safery check already treat it as unsafe, we do not need to
anything about it.)
Except for the above cases, we treat other cases as parallel safe.
---
src/backend/optimizer/util/clauses.c | 84 +++++++++++++++++++++++++++++++++++-
src/include/catalog/pg_proc.dat | 8 ++--
2 files changed, 87 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index ade66a5..cff18dd 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -100,6 +100,7 @@ typedef struct
RangeTblEntry *target_rte; /* query's target relation if any */
CmdType command_type; /* query's command type */
PlannerGlobal *planner_global; /* global info for planner invocation */
+ List *pk_rels; /* OIDs of relations referenced by target relation */
} max_parallel_hazard_context;
static bool contain_agg_clause_walker(Node *node, void *context);
@@ -582,6 +583,7 @@ max_parallel_hazard(Query *parse, PlannerGlobal *glob)
rt_fetch(parse->resultRelation, parse->rtable) : NULL;
context.command_type = parse->commandType;
context.planner_global = glob;
+ context.pk_rels = NIL;
(void) max_parallel_hazard_walker((Node *) parse, &context);
return context.max_hazard;
@@ -873,7 +875,7 @@ static bool
target_rel_max_parallel_hazard(max_parallel_hazard_context *context)
{
bool max_hazard_found;
-
+ ListCell *lc;
Relation targetRel;
/*
@@ -884,6 +886,36 @@ target_rel_max_parallel_hazard(max_parallel_hazard_context *context)
max_hazard_found = target_rel_max_parallel_hazard_recurse(targetRel,
context->command_type,
context);
+
+ /*
+ * Check if target relation is one of PK relations.
+ */
+ if (!max_hazard_found && context->max_hazard == PROPARALLEL_SAFE)
+ {
+ if (list_member_oid(context->pk_rels, context->target_rte->relid))
+ {
+ max_hazard_found = max_parallel_hazard_test(PROPARALLEL_RESTRICTED,
+ context);
+ }
+ }
+
+ /*
+ * check if any pk relation is partition of the target table.
+ */
+ if (context->max_hazard == PROPARALLEL_SAFE)
+ {
+ foreach(lc, context->pk_rels)
+ {
+ if (list_member_oid(context->planner_global->partitionOids,
+ lfirst_oid(lc)))
+ {
+ max_hazard_found = max_parallel_hazard_test(PROPARALLEL_RESTRICTED,
+ context);
+ break;
+ }
+ }
+ }
+
table_close(targetRel, NoLock);
return max_hazard_found;
@@ -972,9 +1004,59 @@ target_rel_trigger_max_parallel_hazard(Relation rel,
for (i = 0; i < rel->trigdesc->numtriggers; i++)
{
Oid tgfoid = rel->trigdesc->triggers[i].tgfoid;
+ int trigtype;
if (max_parallel_hazard_test(func_parallel(tgfoid), context))
return true;
+
+ /*
+ * If the trigger type is RI_TRIGGER_FK, this indicates a FK exists in
+ * the relation, this should result in creation of new CommandIds if
+ * PK relation is modified.
+ *
+ * However, creation of new CommandIds is not supported in a
+ * parallel worker(but is safe in the parallel leader). So, treat all
+ * scenarios that may modify PK relation as parallel-restricted.
+ */
+ trigtype = RI_FKey_trigger_type(tgfoid);
+ if (trigtype == RI_TRIGGER_FK)
+ {
+ HeapTuple tup;
+ Form_pg_constraint conForm;
+ Oid constraintOid;
+
+ constraintOid = rel->trigdesc->triggers[i].tgconstraint;
+
+ /*
+ * Fetch the pg_constraint row so we can fill in the entry.
+ */
+ tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraintOid));
+ if (!HeapTupleIsValid(tup)) /* should not happen */
+ elog(ERROR, "cache lookup failed for constraint %u",
+ constraintOid);
+
+ conForm = (Form_pg_constraint) GETSTRUCT(tup);
+ if (conForm->contype != CONSTRAINT_FOREIGN) /* should not happen */
+ elog(ERROR, "constraint %u is not a foreign key constraint",
+ constraintOid);
+
+ /*
+ * If FK relation and PK relation are not the same,
+ * they could be partitions of the same table.
+ * Remember Oids of PK relation for the later check.
+ */
+ if (conForm->confrelid != conForm->conrelid)
+ context->pk_rels = lappend_oid(context->pk_rels, conForm->confrelid);
+
+ /*
+ * If FK relation and PK relation are the same, the PK relation
+ * could be modfied.
+ */
+ else if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
+ return true;
+
+ ReleaseSysCache(tup);
+ }
}
return false;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 93393fc..61361a6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3745,11 +3745,11 @@
# Generic referential integrity constraint triggers
{ oid => '1644', descr => 'referential integrity FOREIGN KEY ... REFERENCES',
- proname => 'RI_FKey_check_ins', provolatile => 'v', proparallel => 'r',
- prorettype => 'trigger', proargtypes => '', prosrc => 'RI_FKey_check_ins' },
+ proname => 'RI_FKey_check_ins', provolatile => 'v', prorettype => 'trigger',
+ proargtypes => '', prosrc => 'RI_FKey_check_ins' },
{ oid => '1645', descr => 'referential integrity FOREIGN KEY ... REFERENCES',
- proname => 'RI_FKey_check_upd', provolatile => 'v', proparallel => 'r',
- prorettype => 'trigger', proargtypes => '', prosrc => 'RI_FKey_check_upd' },
+ proname => 'RI_FKey_check_upd', provolatile => 'v', prorettype => 'trigger',
+ proargtypes => '', prosrc => 'RI_FKey_check_upd' },
{ oid => '1646', descr => 'referential integrity ON DELETE CASCADE',
proname => 'RI_FKey_cascade_del', provolatile => 'v', prorettype => 'trigger',
proargtypes => '', prosrc => 'RI_FKey_cascade_del' },
--
2.7.2.windows.1
v2-0001-skip-cci.patchapplication/octet-stream; name=v2-0001-skip-cci.patchDownload
From abadf6b0848bab6fae37b6e8bfba377fb44019ec Mon Sep 17 00:00:00 2001
From: houzj <houzj.fnst@cn.fujitsu.com>
Date: Tue, 16 Mar 2021 14:57:07 +0800
Subject: [PATCH] skip cci
In RI check, we currently call CommandCounterIncrement everytime when insert into
table with foreign which is not supported in parallel worker. However, It's necessary
to do CCI only if referenced table is modified during INSERT command.
For now, all the cases that will modify the referenced table is treated as parallel unsafe.
We can skip CCI to let RI check(for now only RI_FKey_check_ins) can be executed in parallel worker.
For the parallel safety of RI_FKey_check_ins,
it internally uses [select pk rel for key share] which is considered as parallel unsafe in planner.
However, [select for key share] does not generate combo-cid or new command id.
And it acquires share lock which does not conflict with itself and works as expected with group locking.
In addition, as we assumed that the pk rel will not be modified, dead lock on pk rel will not happen.
Based on above, IMO, execute [select for key share] in parallel worker is as safe as parallel insert.
---
src/backend/executor/spi.c | 28 ++++++++++++----------
src/backend/utils/adt/ri_triggers.c | 48 ++++++++++++++++++++++++++-----------
src/include/executor/spi.h | 3 ++-
3 files changed, 52 insertions(+), 27 deletions(-)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 00aa78e..f1dd775 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -69,7 +69,8 @@ static int _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
bool read_only, bool no_snapshots,
bool fire_triggers, uint64 tcount,
DestReceiver *caller_dest,
- ResourceOwner plan_owner);
+ ResourceOwner plan_owner,
+ bool no_CCI);
static ParamListInfo _SPI_convert_params(int nargs, Oid *argtypes,
Datum *Values, const char *Nulls);
@@ -525,7 +526,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, false);
_SPI_end_call(true);
return res;
@@ -569,7 +570,7 @@ SPI_execute_extended(const char *src,
InvalidSnapshot, InvalidSnapshot,
options->read_only, options->no_snapshots,
true, options->tcount,
- options->dest, options->owner);
+ options->dest, options->owner, false);
_SPI_end_call(true);
return res;
@@ -598,7 +599,7 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, false);
_SPI_end_call(true);
return res;
@@ -629,7 +630,7 @@ SPI_execute_plan_extended(SPIPlanPtr plan,
InvalidSnapshot, InvalidSnapshot,
options->read_only, options->no_snapshots,
true, options->tcount,
- options->dest, options->owner);
+ options->dest, options->owner, false);
_SPI_end_call(true);
return res;
@@ -653,7 +654,7 @@ SPI_execute_plan_with_paramlist(SPIPlanPtr plan, ParamListInfo params,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, false);
_SPI_end_call(true);
return res;
@@ -676,7 +677,8 @@ int
SPI_execute_snapshot(SPIPlanPtr plan,
Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
- bool read_only, bool fire_triggers, long tcount)
+ bool read_only, bool fire_triggers, long tcount,
+ bool no_CCI)
{
int res;
@@ -696,7 +698,7 @@ SPI_execute_snapshot(SPIPlanPtr plan,
snapshot, crosscheck_snapshot,
read_only, false,
fire_triggers, tcount,
- NULL, NULL);
+ NULL, NULL, no_CCI);
_SPI_end_call(true);
return res;
@@ -746,7 +748,7 @@ SPI_execute_with_args(const char *src,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, false);
_SPI_end_call(true);
return res;
@@ -2271,13 +2273,15 @@ _SPI_prepare_oneshot_plan(const char *src, SPIPlanPtr plan)
* caller_dest: DestReceiver to receive output, or NULL for normal SPI output
* plan_owner: ResourceOwner that will be used to hold refcount on plan;
* if NULL, CurrentResourceOwner is used (ignored for non-saved plan)
+ * no_CCI: true to skip CommandCounterIncrement even if read_only is false
*/
static int
_SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
Snapshot snapshot, Snapshot crosscheck_snapshot,
bool read_only, bool no_snapshots,
bool fire_triggers, uint64 tcount,
- DestReceiver *caller_dest, ResourceOwner plan_owner)
+ DestReceiver *caller_dest, ResourceOwner plan_owner,
+ bool no_CCI)
{
int my_res = 0;
uint64 my_processed = 0;
@@ -2464,7 +2468,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
* If not read-only mode, advance the command counter before each
* command and update the snapshot.
*/
- if (!read_only && !no_snapshots)
+ if (!no_CCI && !read_only && !no_snapshots)
{
CommandCounterIncrement();
UpdateActiveSnapshotCommandId();
@@ -2608,7 +2612,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
* command. This ensures that its effects are visible, in case it was
* DDL that would affect the next CachedPlanSource.
*/
- if (!read_only)
+ if (!no_CCI && !read_only)
CommandCounterIncrement();
}
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 09a2ad2..959ff1f 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -217,7 +217,7 @@ static bool ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
TupleTableSlot *oldslot, TupleTableSlot *newslot,
- bool detectNewRows, int expect_OK);
+ bool detectNewRows, int expect_OK, bool modifypk);
static void ri_ExtractValues(Relation rel, TupleTableSlot *slot,
const RI_ConstraintInfo *riinfo, bool rel_is_pk,
Datum *vals, char *nulls);
@@ -233,7 +233,7 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo,
* Check foreign key existence (combined for INSERT and UPDATE).
*/
static Datum
-RI_FKey_check(TriggerData *trigdata)
+RI_FKey_check(TriggerData *trigdata, bool modifypk)
{
const RI_ConstraintInfo *riinfo;
Relation fk_rel;
@@ -397,7 +397,8 @@ RI_FKey_check(TriggerData *trigdata)
fk_rel, pk_rel,
NULL, newslot,
false,
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ modifypk);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -416,11 +417,25 @@ RI_FKey_check(TriggerData *trigdata)
Datum
RI_FKey_check_ins(PG_FUNCTION_ARGS)
{
+ bool modifypk = true;
+
/* Check that this is a valid trigger call on the right time and event. */
ri_CheckTrigger(fcinfo, "RI_FKey_check_ins", RI_TRIGTYPE_INSERT);
+ /*
+ * We do not allow parallel insert if the pk relation could be modified
+ * during INSERT command, because it needs to call CommandCounterIncrement
+ * to let the modifications on pk relation visible for the RI check which
+ * is not support in parallel worker.
+ *
+ * (Note: Once we support CCI in parallel worker, this dependency is no
+ * longer needed).
+ */
+ if (IsInParallelMode())
+ modifypk = false;
+
/* Share code with UPDATE case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, modifypk);
}
@@ -436,7 +451,7 @@ RI_FKey_check_upd(PG_FUNCTION_ARGS)
ri_CheckTrigger(fcinfo, "RI_FKey_check_upd", RI_TRIGTYPE_UPDATE);
/* Share code with INSERT case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, true);
}
@@ -524,7 +539,8 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
fk_rel, pk_rel,
oldslot, NULL,
true, /* treat like update */
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -716,7 +732,8 @@ ri_restrict(TriggerData *trigdata, bool is_no_action)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -822,7 +839,8 @@ RI_FKey_cascade_del(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_DELETE);
+ SPI_OK_DELETE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -943,7 +961,8 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
oldslot, newslot,
true, /* must detect new rows */
- SPI_OK_UPDATE);
+ SPI_OK_UPDATE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -1122,7 +1141,8 @@ ri_set(TriggerData *trigdata, bool is_set_null)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_UPDATE);
+ SPI_OK_UPDATE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -1496,7 +1516,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
NULL, NULL,
GetLatestSnapshot(),
InvalidSnapshot,
- true, false, 1);
+ true, false, 1, false);
/* Check result */
if (spi_result != SPI_OK_SELECT)
@@ -1736,7 +1756,7 @@ RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
NULL, NULL,
GetLatestSnapshot(),
InvalidSnapshot,
- true, false, 1);
+ true, false, 1, false);
/* Check result */
if (spi_result != SPI_OK_SELECT)
@@ -2238,7 +2258,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
TupleTableSlot *oldslot, TupleTableSlot *newslot,
- bool detectNewRows, int expect_OK)
+ bool detectNewRows, int expect_OK, bool modifypk)
{
Relation query_rel,
source_rel;
@@ -2336,7 +2356,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
spi_result = SPI_execute_snapshot(qplan,
vals, nulls,
test_snapshot, crosscheck_snapshot,
- false, false, limit);
+ false, false, limit, !modifypk);
/* Restore UID and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index 6455d10..7150199 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -129,7 +129,8 @@ extern int SPI_execute_snapshot(SPIPlanPtr plan,
Datum *Values, const char *Nulls,
Snapshot snapshot,
Snapshot crosscheck_snapshot,
- bool read_only, bool fire_triggers, long tcount);
+ bool read_only, bool fire_triggers, long tcount,
+ bool no_CCI);
extern int SPI_execute_with_args(const char *src,
int nargs, Oid *argtypes,
Datum *Values, const char *Nulls,
--
2.7.2.windows.1
On Tue, Mar 16, 2021 at 8:41 PM houzj.fnst@fujitsu.com <
houzj.fnst@fujitsu.com> wrote:
To support parallel insert into FK relation.
There are two scenarios need attention.
1) foreign key and primary key are on the same table(INSERT's target
table).
(referenced and referencing are the same table)
2) referenced and referencing table are both partition of INSERT's
target table.
(These cases are really rare for me)
In the two cases, the referenced table could be modified when INSERTing
and
CCI is necessary, So, I think we should treat these cases as parallel
restricted
while doing safety check.
Attaching V1 patch that Implemented above feature and passed regression
test.Attaching rebased patch based on HEAD.
I noticed some things on the first scan through:
Patch 0001:
1) Tidy up the comments a bit:
Suggest the following update to part of the comments:
In RI check, we currently call CommandCounterIncrement every time we insert
into
a table with foreign key, which is not supported in a parallel worker.
However, it's necessary
to do CCI only if the referenced table is modified during an INSERT command.
For now, all the cases that will modify the referenced table are treated as
parallel unsafe.
We can skip CCI to let the RI check (for now only RI_FKey_check_ins) to be
executed in a parallel worker.
Patch 0002:
1) The new max_parallel_hazard_context member "pk_rels" is not being set
(to NIL) in the is_parallel_safe() function, so it will have a junk value
in that case - though it does look like nothing could reference it then
(but the issue may be detected by a Valgrind build, as performed by the
buildfarm).
2) Few things to tidy up the patch comments:
i)
Currently, We can not support parallel insert into fk relation in all cases.
should be:
Currently, we cannot support parallel insert into a fk relation in all
cases.
ii)
When inserting into a table with foreign key, if the referenced could also
be modified in
INSERT command, we will need to do CommandCounterIncrement to let the
modification
on referenced table visible for the RI check which is not supported in
parallel worker.
should be:
When inserting into a table with a foreign key, if the referenced table can
also be modified by
the INSERT command, we will need to do CommandCounterIncrement to let the
modification
on the referenced table be visible for the RI check, which is not supported
in a parallel worker.
iii)
So, Extent the parallel-safety check to treat the following cases(could
modify referenced table) parallel restricted:
should be:
So, extend the parallel-safety check to treat the following cases (could
modify referenced table) as parallel restricted:
iv)
However, the current parallel safery check already treat it as unsafe, we
do not need to
anything about it.)
should be:
However, the current parallel safety checks already treat it as unsafe, so
we do not need to
do anything about it.)
3) In target_rel_trigger_max_parallel_hazard(), you have added a variable
declaration "int trigtype;" after code, instead of before:
Oid tgfoid = rel->trigdesc->triggers[i].tgfoid;
+ int trigtype;
should be:
+ int trigtype;
Oid tgfoid = rel->trigdesc->triggers[i].tgfoid;
(need to avoid intermingled declarations and code)
See: https://www.postgresql.org/docs/13/source-conventions.html
Regards,
Greg Nancarrow
Fujitsu Australia
Hi,
Thanks for the review.
I noticed some things on the first scan through:
Patch 0001:
1) Tidy up the comments a bit:Suggest the following update to part of the comments:
Changed.
Patch 0002:
1) The new max_parallel_hazard_context member "pk_rels" is not being
set (to
NIL) in the is_parallel_safe() function, so it will have a junk value
in that case - though it does look like nothing could reference it
then (but the issue may be detected by a Valgrind build, as performed by the buildfarm).
Changed.
2) Few things to tidy up the patch comments:
Changed.
3) In target_rel_trigger_max_parallel_hazard(), you have added a
variable declaration "int trigtype;" after code, instead of before:
Changed.
Attaching new version patch with these changes.
Also attaching the simple performance test results (insert into table with foreign key):
postgres=# explain (analyze, verbose) insert into fk select a,func_xxx() from data where a%2=0 or a%3 = 0;
workers | serial insert + parallel select | parallel insert | performace gain
-----------+---------------------------------+-----------------+--------
2 workers | 85512.153ms | 61384.957ms | 29%
4 workers | 85436.957ms | 39335.797ms | 54%
-------------data prepare-----------------------------
create table pk(a int primary key,b int);
create table fk(a int references pk,b int);
create table data(a int, b int);
insert into data select * from generate_series(1,10000000,1) t;
insert into pk select generate_series(1,10000000,1);
------------------------------------------------
Best regards,
houzj
Attachments:
v3-0001-skip-cci.patchapplication/octet-stream; name=v3-0001-skip-cci.patchDownload
From abadf6b0848bab6fae37b6e8bfba377fb44019ec Mon Sep 17 00:00:00 2001
From: houzj <houzj.fnst@cn.fujitsu.com>
Date: Tue, 16 Mar 2021 14:57:07 +0800
Subject: [PATCH] skip cci
In RI check, we currently call CommandCounterIncrement every time we insert into
a table with foreign key, which is not supported in a parallel worker. However, it's necessary
to do CCI only if the referenced table is modified during an INSERT command.
For now, all the cases that will modify the referenced table are treated as parallel unsafe.
We can skip CCI to let the RI check (for now only RI_FKey_check_ins) to be executed in a parallel worker.
For the parallel safety of RI_FKey_check_ins,
it internally uses [select pk rel for key share] which is considered as parallel unsafe in planner.
However, [select for key share] does not generate combo-cid or new command id.
And it acquires share lock which does not conflict with itself and works as expected with group locking.
In addition, as we assumed that the pk rel will not be modified, dead lock on pk rel will not happen.
Based on above, IMO, execute [select for key share] in parallel worker is as safe as parallel insert.
---
src/backend/executor/spi.c | 28 ++++++++++++----------
src/backend/utils/adt/ri_triggers.c | 48 ++++++++++++++++++++++++++-----------
src/include/executor/spi.h | 3 ++-
3 files changed, 52 insertions(+), 27 deletions(-)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 00aa78e..f1dd775 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -69,7 +69,8 @@ static int _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
bool read_only, bool no_snapshots,
bool fire_triggers, uint64 tcount,
DestReceiver *caller_dest,
- ResourceOwner plan_owner);
+ ResourceOwner plan_owner,
+ bool no_CCI);
static ParamListInfo _SPI_convert_params(int nargs, Oid *argtypes,
Datum *Values, const char *Nulls);
@@ -525,7 +526,7 @@ SPI_execute(const char *src, bool read_only, long tcount)
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, false);
_SPI_end_call(true);
return res;
@@ -569,7 +570,7 @@ SPI_execute_extended(const char *src,
InvalidSnapshot, InvalidSnapshot,
options->read_only, options->no_snapshots,
true, options->tcount,
- options->dest, options->owner);
+ options->dest, options->owner, false);
_SPI_end_call(true);
return res;
@@ -598,7 +599,7 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, false);
_SPI_end_call(true);
return res;
@@ -629,7 +630,7 @@ SPI_execute_plan_extended(SPIPlanPtr plan,
InvalidSnapshot, InvalidSnapshot,
options->read_only, options->no_snapshots,
true, options->tcount,
- options->dest, options->owner);
+ options->dest, options->owner, false);
_SPI_end_call(true);
return res;
@@ -653,7 +654,7 @@ SPI_execute_plan_with_paramlist(SPIPlanPtr plan, ParamListInfo params,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, false);
_SPI_end_call(true);
return res;
@@ -676,7 +677,8 @@ int
SPI_execute_snapshot(SPIPlanPtr plan,
Datum *Values, const char *Nulls,
Snapshot snapshot, Snapshot crosscheck_snapshot,
- bool read_only, bool fire_triggers, long tcount)
+ bool read_only, bool fire_triggers, long tcount,
+ bool no_CCI)
{
int res;
@@ -696,7 +698,7 @@ SPI_execute_snapshot(SPIPlanPtr plan,
snapshot, crosscheck_snapshot,
read_only, false,
fire_triggers, tcount,
- NULL, NULL);
+ NULL, NULL, no_CCI);
_SPI_end_call(true);
return res;
@@ -746,7 +748,7 @@ SPI_execute_with_args(const char *src,
InvalidSnapshot, InvalidSnapshot,
read_only, false,
true, tcount,
- NULL, NULL);
+ NULL, NULL, false);
_SPI_end_call(true);
return res;
@@ -2271,13 +2273,15 @@ _SPI_prepare_oneshot_plan(const char *src, SPIPlanPtr plan)
* caller_dest: DestReceiver to receive output, or NULL for normal SPI output
* plan_owner: ResourceOwner that will be used to hold refcount on plan;
* if NULL, CurrentResourceOwner is used (ignored for non-saved plan)
+ * no_CCI: true to skip CommandCounterIncrement even if read_only is false
*/
static int
_SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
Snapshot snapshot, Snapshot crosscheck_snapshot,
bool read_only, bool no_snapshots,
bool fire_triggers, uint64 tcount,
- DestReceiver *caller_dest, ResourceOwner plan_owner)
+ DestReceiver *caller_dest, ResourceOwner plan_owner,
+ bool no_CCI)
{
int my_res = 0;
uint64 my_processed = 0;
@@ -2464,7 +2468,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
* If not read-only mode, advance the command counter before each
* command and update the snapshot.
*/
- if (!read_only && !no_snapshots)
+ if (!no_CCI && !read_only && !no_snapshots)
{
CommandCounterIncrement();
UpdateActiveSnapshotCommandId();
@@ -2608,7 +2612,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
* command. This ensures that its effects are visible, in case it was
* DDL that would affect the next CachedPlanSource.
*/
- if (!read_only)
+ if (!no_CCI && !read_only)
CommandCounterIncrement();
}
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 09a2ad2..959ff1f 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -217,7 +217,7 @@ static bool ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
TupleTableSlot *oldslot, TupleTableSlot *newslot,
- bool detectNewRows, int expect_OK);
+ bool detectNewRows, int expect_OK, bool modifypk);
static void ri_ExtractValues(Relation rel, TupleTableSlot *slot,
const RI_ConstraintInfo *riinfo, bool rel_is_pk,
Datum *vals, char *nulls);
@@ -233,7 +233,7 @@ static void ri_ReportViolation(const RI_ConstraintInfo *riinfo,
* Check foreign key existence (combined for INSERT and UPDATE).
*/
static Datum
-RI_FKey_check(TriggerData *trigdata)
+RI_FKey_check(TriggerData *trigdata, bool modifypk)
{
const RI_ConstraintInfo *riinfo;
Relation fk_rel;
@@ -397,7 +397,8 @@ RI_FKey_check(TriggerData *trigdata)
fk_rel, pk_rel,
NULL, newslot,
false,
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ modifypk);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -416,11 +417,25 @@ RI_FKey_check(TriggerData *trigdata)
Datum
RI_FKey_check_ins(PG_FUNCTION_ARGS)
{
+ bool modifypk = true;
+
/* Check that this is a valid trigger call on the right time and event. */
ri_CheckTrigger(fcinfo, "RI_FKey_check_ins", RI_TRIGTYPE_INSERT);
+ /*
+ * We do not allow parallel insert if the pk relation could be modified
+ * during INSERT command, because it needs to call CommandCounterIncrement
+ * to let the modifications on pk relation visible for the RI check which
+ * is not support in parallel worker.
+ *
+ * (Note: Once we support CCI in parallel worker, this dependency is no
+ * longer needed).
+ */
+ if (IsInParallelMode())
+ modifypk = false;
+
/* Share code with UPDATE case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, modifypk);
}
@@ -436,7 +451,7 @@ RI_FKey_check_upd(PG_FUNCTION_ARGS)
ri_CheckTrigger(fcinfo, "RI_FKey_check_upd", RI_TRIGTYPE_UPDATE);
/* Share code with INSERT case. */
- return RI_FKey_check((TriggerData *) fcinfo->context);
+ return RI_FKey_check((TriggerData *) fcinfo->context, true);
}
@@ -524,7 +539,8 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
fk_rel, pk_rel,
oldslot, NULL,
true, /* treat like update */
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -716,7 +732,8 @@ ri_restrict(TriggerData *trigdata, bool is_no_action)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_SELECT);
+ SPI_OK_SELECT,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -822,7 +839,8 @@ RI_FKey_cascade_del(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_DELETE);
+ SPI_OK_DELETE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -943,7 +961,8 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
oldslot, newslot,
true, /* must detect new rows */
- SPI_OK_UPDATE);
+ SPI_OK_UPDATE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -1122,7 +1141,8 @@ ri_set(TriggerData *trigdata, bool is_set_null)
fk_rel, pk_rel,
oldslot, NULL,
true, /* must detect new rows */
- SPI_OK_UPDATE);
+ SPI_OK_UPDATE,
+ true);
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
@@ -1496,7 +1516,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
NULL, NULL,
GetLatestSnapshot(),
InvalidSnapshot,
- true, false, 1);
+ true, false, 1, false);
/* Check result */
if (spi_result != SPI_OK_SELECT)
@@ -1736,7 +1756,7 @@ RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
NULL, NULL,
GetLatestSnapshot(),
InvalidSnapshot,
- true, false, 1);
+ true, false, 1, false);
/* Check result */
if (spi_result != SPI_OK_SELECT)
@@ -2238,7 +2258,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
TupleTableSlot *oldslot, TupleTableSlot *newslot,
- bool detectNewRows, int expect_OK)
+ bool detectNewRows, int expect_OK, bool modifypk)
{
Relation query_rel,
source_rel;
@@ -2336,7 +2356,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
spi_result = SPI_execute_snapshot(qplan,
vals, nulls,
test_snapshot, crosscheck_snapshot,
- false, false, limit);
+ false, false, limit, !modifypk);
/* Restore UID and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index 6455d10..7150199 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -129,7 +129,8 @@ extern int SPI_execute_snapshot(SPIPlanPtr plan,
Datum *Values, const char *Nulls,
Snapshot snapshot,
Snapshot crosscheck_snapshot,
- bool read_only, bool fire_triggers, long tcount);
+ bool read_only, bool fire_triggers, long tcount,
+ bool no_CCI);
extern int SPI_execute_with_args(const char *src,
int nargs, Oid *argtypes,
Datum *Values, const char *Nulls,
--
2.7.2.windows.1
v3-0002-extend-safery-check-for-fk-relation.patchapplication/octet-stream; name=v3-0002-extend-safery-check-for-fk-relation.patchDownload
From a0976847db01a5160e0915b5f2127fd0ada10f49 Mon Sep 17 00:00:00 2001
From: houzj <houzj.fnst@cn.fujitsu.com>
Date: Mon, 22 Mar 2021 09:49:00 +0800
Subject: [PATCH] extend safery check to support parallel insert into fk
relation
Currently, we cannot support parallel insert into a fk relation in all cases.
For example:
When inserting into a table with a foreign key, if the referenced table can also be modified by
the INSERT command, we will need to do CommandCounterIncrement to let the modification
on the referenced table be visible for the RI check, which is not supported in a parallel worker.
So, extend the parallel-safety check to treat the following cases (could modify referenced table) as parallel restricted:
1) foreign key and primary key are on the same table(INSERT's target table).
(referenced and referencing are the same table)
2) referenced and referencing table are both partition of INSERT's target table.
(Note: modifyingCTE and function with modifying statement could also modifiy the referenced table,
However, the current parallel safety checks already treat it as unsafe, so we do not need to
do anything about it.)
Except for the above cases, we treat other cases as parallel safe.
---
src/backend/optimizer/util/clauses.c | 85 +++++++++++++++++++++++++++++++++++-
src/include/catalog/pg_proc.dat | 8 ++--
2 files changed, 88 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index c6be4f8..b8bd0b0 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -100,6 +100,7 @@ typedef struct
RangeTblEntry *target_rte; /* query's target relation if any */
CmdType command_type; /* query's command type */
PlannerGlobal *planner_global; /* global info for planner invocation */
+ List *pk_rels; /* OIDs of relations referenced by target relation */
} max_parallel_hazard_context;
static bool contain_agg_clause_walker(Node *node, void *context);
@@ -582,6 +583,7 @@ max_parallel_hazard(Query *parse, PlannerGlobal *glob)
rt_fetch(parse->resultRelation, parse->rtable) : NULL;
context.command_type = parse->commandType;
context.planner_global = glob;
+ context.pk_rels = NIL;
(void) max_parallel_hazard_walker((Node *) parse, &context);
return context.max_hazard;
@@ -618,6 +620,7 @@ is_parallel_safe(PlannerInfo *root, Node *node)
context.target_rte = NULL;
context.command_type = CMD_UNKNOWN;
context.planner_global = NULL;
+ context.pk_rels = NIL;
/*
* The params that refer to the same or parent query level are considered
@@ -873,7 +876,7 @@ static bool
target_rel_max_parallel_hazard(max_parallel_hazard_context *context)
{
bool max_hazard_found;
-
+ ListCell *lc;
Relation targetRel;
/*
@@ -884,6 +887,36 @@ target_rel_max_parallel_hazard(max_parallel_hazard_context *context)
max_hazard_found = target_rel_max_parallel_hazard_recurse(targetRel,
context->command_type,
context);
+
+ /*
+ * Check if target relation is one of PK relations.
+ */
+ if (!max_hazard_found && context->max_hazard == PROPARALLEL_SAFE)
+ {
+ if (list_member_oid(context->pk_rels, context->target_rte->relid))
+ {
+ max_hazard_found = max_parallel_hazard_test(PROPARALLEL_RESTRICTED,
+ context);
+ }
+ }
+
+ /*
+ * check if any pk relation is partition of the target table.
+ */
+ if (context->max_hazard == PROPARALLEL_SAFE)
+ {
+ foreach(lc, context->pk_rels)
+ {
+ if (list_member_oid(context->planner_global->partitionOids,
+ lfirst_oid(lc)))
+ {
+ max_hazard_found = max_parallel_hazard_test(PROPARALLEL_RESTRICTED,
+ context);
+ break;
+ }
+ }
+ }
+
table_close(targetRel, NoLock);
return max_hazard_found;
@@ -971,10 +1004,60 @@ target_rel_trigger_max_parallel_hazard(Relation rel,
*/
for (i = 0; i < rel->trigdesc->numtriggers; i++)
{
+ int trigtype;
Oid tgfoid = rel->trigdesc->triggers[i].tgfoid;
if (max_parallel_hazard_test(func_parallel(tgfoid), context))
return true;
+
+ /*
+ * If the trigger type is RI_TRIGGER_FK, this indicates a FK exists in
+ * the relation, this should result in creation of new CommandIds if
+ * PK relation is modified.
+ *
+ * However, creation of new CommandIds is not supported in a
+ * parallel worker(but is safe in the parallel leader). So, treat all
+ * scenarios that may modify PK relation as parallel-restricted.
+ */
+ trigtype = RI_FKey_trigger_type(tgfoid);
+ if (trigtype == RI_TRIGGER_FK)
+ {
+ HeapTuple tup;
+ Form_pg_constraint conForm;
+ Oid constraintOid;
+
+ constraintOid = rel->trigdesc->triggers[i].tgconstraint;
+
+ /*
+ * Fetch the pg_constraint row so we can fill in the entry.
+ */
+ tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraintOid));
+ if (!HeapTupleIsValid(tup)) /* should not happen */
+ elog(ERROR, "cache lookup failed for constraint %u",
+ constraintOid);
+
+ conForm = (Form_pg_constraint) GETSTRUCT(tup);
+ if (conForm->contype != CONSTRAINT_FOREIGN) /* should not happen */
+ elog(ERROR, "constraint %u is not a foreign key constraint",
+ constraintOid);
+
+ /*
+ * If FK relation and PK relation are not the same,
+ * they could be partitions of the same table.
+ * Remember Oids of PK relation for the later check.
+ */
+ if (conForm->confrelid != conForm->conrelid)
+ context->pk_rels = lappend_oid(context->pk_rels, conForm->confrelid);
+
+ /*
+ * If FK relation and PK relation are the same, the PK relation
+ * could be modfied.
+ */
+ else if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
+ return true;
+
+ ReleaseSysCache(tup);
+ }
}
return false;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index e259531..64db87e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3745,11 +3745,11 @@
# Generic referential integrity constraint triggers
{ oid => '1644', descr => 'referential integrity FOREIGN KEY ... REFERENCES',
- proname => 'RI_FKey_check_ins', provolatile => 'v', proparallel => 'r',
- prorettype => 'trigger', proargtypes => '', prosrc => 'RI_FKey_check_ins' },
+ proname => 'RI_FKey_check_ins', provolatile => 'v', prorettype => 'trigger',
+ proargtypes => '', prosrc => 'RI_FKey_check_ins' },
{ oid => '1645', descr => 'referential integrity FOREIGN KEY ... REFERENCES',
- proname => 'RI_FKey_check_upd', provolatile => 'v', proparallel => 'r',
- prorettype => 'trigger', proargtypes => '', prosrc => 'RI_FKey_check_upd' },
+ proname => 'RI_FKey_check_upd', provolatile => 'v', prorettype => 'trigger',
+ proargtypes => '', prosrc => 'RI_FKey_check_upd' },
{ oid => '1646', descr => 'referential integrity ON DELETE CASCADE',
proname => 'RI_FKey_cascade_del', provolatile => 'v', prorettype => 'trigger',
proargtypes => '', prosrc => 'RI_FKey_cascade_del' },
--
2.7.2.windows.1