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(at)gmail(dot)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
>