On Thu, Jan 21, 2021 at 1:28 PM Zhihong Yu <z...@yugabyte.com> wrote:
>
> Hi,
> For v12-0001-Enable-parallel-SELECT-for-INSERT-INTO-.-SELECT.patch :
>
> is found from the additional parallel-safety checks, or from the existing
> parallel-safety checks for SELECT that it currently performs.
>
> existing and 'it currently performs' are redundant. You can omit 'that it
> currently performs'.
>
OK, but this is very minor.
> Minor. For index_expr_max_parallel_hazard_for_modify(),
>
> + if (keycol == 0)
> + {
> + /* Found an index expression */
>
> You can check if keycol != 0, continue with the loop. This would save some
> indent.
Yes I know, but I don't really see any issue with indent (I'm using
4-space tabs).
>
> + if (index_expr_item == NULL) /* shouldn't happen */
> + {
> + elog(WARNING, "too few entries in indexprs list");
>
> I think the warning should be an error since there is assertion ahead of the
> if statement.
>
Assertions are normally for DEBUG builds, so the Assert would have no
effect in a production (release) build.
Besides, as I have explained in my reply to previous feedback, the
logging of a WARNING (rather than ERROR) is intentional, because I
want processing to continue (not stop) if ever this very rare
condition was to occur - so that the issue can be dealt with by the
current non-parallel processing (rather than stop dead in the middle
of parallel-safety-checking code). For a DEBUG build, it is handy for
the Assert to immediately alert us to the issue (which could really
only be caused by a database corruption, not bug in the code).
Note that Vignesh originally suggested changing it from
"elog(ERROR,...)" to "elog(WARNING,...)", and I agree with him.
Regards,
Greg Nancarrow
Fujitsu Australia
