Re: Parallel INSERT (INTO ... SELECT ...)

On Mon, Jan 25, 2021 at 2:22 PM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com>
wrote:

> Hi,
>
> After doing some test to cover the code path in the PATCH 0001.
> I have some suggestions for the 0002 testcase.
>
>
> (1)
> + /* Check parallel-safety of any expressions in the
> partition key */
> + if (get_partition_col_attnum(pkey, i) == 0)
> + {
> + Node *check_expr = (Node *)
> lfirst(partexprs_item);
> +
> + if (max_parallel_hazard_walker(check_expr,
> context))
> + {
> + table_close(rel, lockmode);
> + return true;
> + }
>
> The testcase seems does not cover the above code(test when the table have
> parallel unsafe expression in the partition key).
>
> Personally, I use the following sql to cover this:
> -----
> create table partkey_unsafe_key_expr_t (a int4, b name) partition by range
> ((fullname_parallel_unsafe('',a::varchar)));
> explain (costs off) insert into partkey_unsafe_key_expr_t select unique1,
> stringu1 from tenk1;
> -----
>
>
Thanks. It looks like that test case was accidently missed (since the
comment said to test the index expressions, but it actually tested the
support functions).
I'll update the test code (and comments) accordingly, using your
suggestion.

>
> (2)
> I noticed that most of testcase test both (parallel
> safe/unsafe/restricted).
> But the index expression seems does not test the parallel restricted.
> How about add a testcase like:
> -----
> create or replace function fullname_parallel_restricted(f text, l text)
> returns text as $$
> begin
> return f || l;
> end;
> $$ language plpgsql immutable parallel restricted;
>
> create table names4(index int, first_name text, last_name text);
> create index names4_fullname_idx on names4
> (fullname_parallel_restricted(first_name, last_name));
>
> --
> -- Test INSERT with parallel-restricted index expression
> -- (should create a parallel plan)
> --
> explain (costs off) insert into names4 select * from names;
> -----
>
>
Thanks, looks like that test case is missing, I'll add it as you suggest.

> (3)
> + /* Recursively check each partition ... */
> + pdesc = RelationGetPartitionDesc(rel);
> + for (i = 0; i < pdesc->nparts; i++)
> + {
> + if
> (rel_max_parallel_hazard_for_modify(pdesc->oids[i],
> +
> command_type,
> +
> context,
> +
> AccessShareLock))
> + {
> + table_close(rel, lockmode);
> + return true;
> + }
> + }
>
> It seems we do not have a testcase to test (some parallel unsafe
> expression or.. in partition)
> Hoe about add one testcase to test parallel unsafe partition ?
>
>
>
OK, I have to create a more complex table to test those other potential
parallel-safety issues of partitions (other than what was tested before the
recursive call, or support functions and expression in index key), but
since it's a recursive call, invoking code that's already been tested, I
would not anticipate any problems.

Thanks,

Greg Nancarrow
Fujitsu Australia