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

>
> See attached patches.
>

Thanks for providing the patches.
I had reviewed
v6-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.patch, please find
my comments:
-> commandType is not used, we can remove it.
+ * Prepare for entering parallel mode by assigning a FullTransactionId, to
be
+ * included in the transaction state that is serialized in the parallel
DSM.
+ */
+void PrepareParallelModeForModify(CmdType commandType)
+{
+ Assert(!IsInParallelMode());
+
+ (void)GetCurrentTransactionId();
+}

-> 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