Re: Parallel INSERT (INTO ... SELECT ...) - Mailing list pgsql-hackers

From Greg Nancarrow
Subject Re: Parallel INSERT (INTO ... SELECT ...)
Date
Msg-id CAJcOf-dW3_q0nB9b9zYrCooqtNo5NmCdwoxKbYMzmCjUU6BUAw@mail.gmail.com
Whole thread Raw
In response to RE: Parallel INSERT (INTO ... SELECT ...)  ("Hou, Zhijie" <houzj.fnst@cn.fujitsu.com>)
List pgsql-hackers

On Mon, Jan 25, 2021 at 2:22 PM Hou, Zhijie <houzj.fnst@cn.fujitsu.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


pgsql-hackers by date:

Previous
From: yuzuko
Date:
Subject: Re: Release SPI plans for referential integrity with DISCARD ALL
Next
From: "Joel Jacobson"
Date:
Subject: The mysterious pg_proc.protrftypes