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

-> As we support insertion of data from the workers, this comments "but as of now, only the leader backend writes into a completely new table.  In the future, we can extend it to allow workers to write into the table" must be updated accordingly:
+        * modify any data using a CTE, or if this is a cursor operation, or if
+        * GUCs are set to values that don't permit parallelism, or if
+        * parallel-unsafe functions are present in the query tree.
         *
-        * (Note that we do allow CREATE TABLE AS, SELECT INTO, and CREATE
+        * (Note that we do allow CREATE TABLE AS, INSERT, SELECT INTO, and CREATE
         * MATERIALIZED VIEW to use parallel plans, but as of now, only the leader
         * backend writes into a completely new table.  In the future, we can
         * extend it to allow workers to write into the table.  However, to allow

-> Also should we specify insert as "insert into select"
 
-> We could include a small writeup of the design may be in the commit message. It will be useful for review.

-> I felt the below two assignment statements can be in the else condition:
                glob->maxParallelHazard = max_parallel_hazard(parse);
                glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);
+
+               /*
+                * Additional parallel-mode safety checks are required in order to
+                * allow an underlying parallel query to be used for a
+                * table-modification command that is supported in parallel-mode.
+                */
+               if (glob->parallelModeOK &&
+                       IsModifySupportedInParallelMode(parse->commandType))
+               {
+                       glob->maxParallelHazard = MaxParallelHazardForModify(parse, &glob->maxParallelHazard);
+                       glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);
+               }

something like:
/*
* Additional parallel-mode safety checks are required in order to
* allow an underlying parallel query to be used for a
* table-modification command that is supported in parallel-mode.
*/
if (glob->parallelModeOK &&
IsModifySupportedInParallelMode(parse->commandType))
glob->maxParallelHazard = MaxParallelHazardForModify(parse, &glob->maxParallelHazard);
else
/* all the cheap tests pass, so scan the query tree */
glob->maxParallelHazard = max_parallel_hazard(parse);
glob->parallelModeOK = (glob->maxParallelHazard != PROPARALLEL_UNSAFE);

-> Comments need slight adjustment, maybe you could run pgindent for the modified code.
+               /*
+                * Supported table-modification commands may require additional steps
+                * prior to entering parallel mode, such as assigning a FullTransactionId.
+                */

-> In the below, max_parallel_hazard_test will return true for PROPARALLEL_RESTRICTED also, Is break intentional in that case? As in case of RI_TRIGGER_FK for PROPARALLEL_RESTRICTED we continue.
+               if (max_parallel_hazard_test(func_parallel(trigger->tgfoid), context))
+                       break;
+
+               /*
+                * If the trigger type is RI_TRIGGER_FK, this indicates a FK exists in
+                * the relation, and this would result in creation of new CommandIds
+                * on insert/update/delete and this isn't supported in a parallel
+                * worker (but is safe in the parallel leader).
+                */
+               trigtype = RI_FKey_trigger_type(trigger->tgfoid);
+               if (trigtype == RI_TRIGGER_FK)
+               {
+                       context->max_hazard = PROPARALLEL_RESTRICTED;
+                       /*
+                        * As we're looking for the max parallel hazard, we don't break
+                        * here; examine any further triggers ...
+                        */
+               }

-> Should we switch to non-parallel mode in this case, instead of throwing error?
+                       val = SysCacheGetAttr(CONSTROID, tup,
+                                               Anum_pg_constraint_conbin, &isnull);
+                       if (isnull)
+                               elog(ERROR, "null conbin for constraint %u", con->oid);
+                       conbin = TextDatumGetCString(val);

-> We could include a few tests for this in regression.

-> We might need some documentation update like in parallel-query.html/parallel-plans.html, etc

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com