Hi
I took a look at v12-0001 patch, here are some comments:
1.
+ /*
+ * Setup the context used in finding the max parallel-mode hazard.
+ */
+ Assert(initial_max_parallel_hazard == 0 ||
+ initial_max_parallel_hazard == PROPARALLEL_SAFE ||
+ initial_max_parallel_hazard == PROPARALLEL_RESTRICTED);
+ context.max_hazard = initial_max_parallel_hazard == 0 ?
+ PROPARALLEL_SAFE : initial_max_parallel_hazard;
I am not quiet sure when will " max_parallel_hazard == 0"
Does it means the case max_parallel_hazard_context not initialized ?
2.
Some tiny code style suggestions
+ if (con->contype == CONSTRAINT_CHECK)
+ {
+ char *conbin;
+ Datum val;
+ bool isnull;
+ Expr *check_expr;
How about :
if (con->contype != CONSTRAINT_CHECK)
continue;
3.
+ if (keycol == 0)
+ {
+ /* Found an index expression */
+
+ Node *index_expr;
Like 2, how about:
If (keycol != 0)
Continue;
4.
+ ListCell *index_expr_item =
list_head(index_info->ii_Expressions);
...
+ index_expr = (Node *)
lfirst(index_expr_item);
+ index_expr = (Node *)
expression_planner((Expr *) index_expr);
It seems BuildIndexInfo has already called eval_const_expressions for
ii_Expressions,
Like the flow: BuildIndexInfo--> RelationGetIndexExpressions-->
eval_const_expressions
So, IMO, we do not need to call expression_planner for the expr again.
And there seems another solution for this:
In the patch, We only use the { ii_Expressions , ii_NumIndexAttrs ,
ii_IndexAttrNumbers } from the IndexInfo,
which seems can get from "Relation-> rd_index".
Based on above, May be we do not need to call BuildIndexInfo to build the
IndexInfo.
It can avoid some unnecessary cost.
And in this solution we do not need to remove expression_planner.
What do you think ?
Best regards,
houzj
