> > 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? Hi
Thanks for the explanation. If the check about command type is only used in assert, I think you are right. I suggested a new function because I guess the check can be used in some other places. Such as: + /* Okay to parallelize inserts, so mark it. */ + if (ins_cmd == PARALLEL_INSERT_CMD_CREATE_TABLE_AS) + ((DR_intorel *) dest)->is_parallel = true; + if (ins_cmd == PARALLEL_INSERT_CMD_CREATE_TABLE_AS) + ((DR_intorel *) dest)->is_parallel = false; Or + if (fpes->ins_cmd_type == PARALLEL_INSERT_CMD_CREATE_TABLE_AS) + pg_atomic_add_fetch_u64(&fpes->processed, queryDesc->estate->es_processed); If you think the above code will extend the ins_cmd type check in the future, the generic function may make sense. Best regards, houzj