Re: Avoid CommandCounterIncrement in RI trigger when INSERT INTO referencing table

On Tue, Mar 16, 2021 at 8:41 PM houzj(dot)fnst(at)fujitsu(dot)com <
houzj(dot)fnst(at)fujitsu(dot)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