> > For v20-0001-Parallel-Inserts-in-CREATE-TABLE-AS.patch :
> >
> > ParallelInsCmdEstimate :
> >
> > + Assert(pcxt && ins_info &&
> > + (ins_cmd == PARALLEL_INSERT_CMD_CREATE_TABLE_AS));
> > +
> > + if (ins_cmd == PARALLEL_INSERT_CMD_CREATE_TABLE_AS)
> >
> > Sinc the if condition is covered by the assertion, I wonder why the if
> check is still needed.
> >
> > Similar comment for SaveParallelInsCmdFixedInfo and
> > SaveParallelInsCmdInfo
>
> Thanks.
>
> The idea is to have assertion with all the expected ins_cmd types, and then
> later to have selective handling for different ins_cmds. For example, if
> we add (in future) parallel insertion in Refresh Materialized View, then
> the code in those functions will be something
> like:
>
> +static void
> +ParallelInsCmdEstimate(ParallelContext *pcxt, ParallelInsertCmdKind
> ins_cmd,
> + void *ins_info)
> +{
> + Assert(pcxt && ins_info &&
> + (ins_cmd == PARALLEL_INSERT_CMD_CREATE_TABLE_AS ||
> + (ins_cmd == PARALLEL_INSERT_CMD_REFRESH_MAT_VIEW));
> +
> + if (ins_cmd == PARALLEL_INSERT_CMD_CREATE_TABLE_AS)
> + {
> +
> + }
> + else if (ns_cmd == PARALLEL_INSERT_CMD_REFRESH_MAT_VIEW)
> + {
> +
> + }
>
> Similarly for other functions as well.
I think it makes sense.
And if the check about ' ins_cmd == xxx1 || ins_cmd == xxx2' may be used in
some places,
How about define a generic function with some comment to mention the purpose.
An example in INSERT INTO SELECT patch:
+/*
+ * IsModifySupportedInParallelMode
+ *
+ * Indicates whether execution of the specified table-modification command
+ * (INSERT/UPDATE/DELETE) in parallel-mode is supported, subject to certain
+ * parallel-safety conditions.
+ */
+static inline bool
+IsModifySupportedInParallelMode(CmdType commandType)
+{
+ /* Currently only INSERT is supported */
+ return (commandType == CMD_INSERT);
+}
Best regards,
houzj
