Re: Avoid CommandCounterIncrement in RI trigger when INSERT INTO referencing table - Mailing list pgsql-hackers
| From | Greg Nancarrow |
|---|---|
| Subject | Re: Avoid CommandCounterIncrement in RI trigger when INSERT INTO referencing table |
| Date | |
| Msg-id | CAJcOf-d7k-K_+_jK=H-Zsptf89uyPp=C+qTHhA=+ZOp-ug8hHQ@mail.gmail.com Whole thread Raw |
| Responses |
RE: Avoid CommandCounterIncrement in RI trigger when INSERT INTO referencing table
("houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>)
|
| List | pgsql-hackers |
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:
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)
Regards,
Greg Nancarrow
Fujitsu Australia
pgsql-hackers by date: