Re: Parallel Inserts in CREATE TABLE AS

On Wed, Jan 6, 2021 at 11:06 AM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote:
>
> > > 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);
> +}

The intention of assert is to verify that those functions are called
for appropriate commands such as CTAS, Refresh Mat View and so on with
correct parameters. I really don't think so we can replace the assert
with a function like above, in the release mode assertion will always
be true. In a way, that assertion is for only debugging purposes. And
I also think that when we as the callers know when to call those new
functions, we can even remove the assertions, if they are really a
problem here. Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com