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

Hi,

For v12-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.patch :

is found from the additional parallel-safety checks, or from the existing
parallel-safety checks for SELECT that it currently performs.

existing and 'it currently performs' are redundant. You can omit 'that it currently performs'.

Minor. For index_expr_max_parallel_hazard_for_modify(), 

+               if (keycol == 0)
+               {
+                   /* Found an index expression */

You can check if keycol != 0, continue with the loop. This would save some indent.

+                   if (index_expr_item == NULL)    /* shouldn't happen */
+                   {
+                       elog(WARNING, "too few entries in indexprs list");

I think the warning should be an error since there is assertion ahead of the if statement.

+           Assert(!isnull);
+           if (isnull)
+           {
+               /*
+                * This shouldn't ever happen, but if it does, log a WARNING
+                * and return UNSAFE, rather than erroring out.
+                */
+               elog(WARNING, "null conbin for constraint %u", con->oid);

The above should be error as well.

Cheers

On Wed, Jan 20, 2021 at 5:06 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

Thanks for the feedback.
Posting an updated set of patches. Changes are based on feedback, as
detailed below:

There's a couple of potential issues currently being looked at:
- locking issues in additional parallel-safety checks?
- apparent uneven work distribution across the parallel workers, for
large insert data


[Antonin]
- Fixed bad Assert in PrepareParallelMode()
- Added missing comment to explain use of GetCurrentCommandId() in
PrepareParallelMode()
- Some variable name shortening in a few places
- Created common function for creation of non-parallel and parallel
ModifyTable paths
- Path count variable changed to bool
- Added FIXME comment to dubious code for creating Gather target-list
from ModifyTable subplan
- Fixed check on returningLists to use NIL instead of NULL

[Amit]
- Moved additional parallel-safety checks (for modify case) into
max_parallel_hazard()
- Removed redundant calls to max_parallel_hazard_test()
- Added Asserts to "should never happen" null-attribute cases (and
added WARNING log missing from one case)
- Added comment for use of NoLock in max_parallel_hazard_for_modify()

[Vignesh]
- Fixed a couple of typos
- Added a couple of test cases for testing that the same transaction
is used by all parallel workers


Regards,
Greg Nancarrow
Fujitsu Australia