Hi Vignesh,
I took a look at the v10 patch set. Here are some comments:
1.
+/*
+ * CheckExprParallelSafety
+ *
+ * Determine if where cluase and default expressions are parallel safe & do not
+ * have volatile expressions, return true if condition satisfies else return
+ * false.
+ */
'cluase' seems a typo.
2.
+ /*
+ * Make sure that no worker has consumed this element, if this
+ * line is spread across multiple data blocks, worker would have
+ * started processing, no need to change the state to
+ * LINE_LEADER_POPULATING in this case.
+ */
+ (void) pg_atomic_compare_exchange_u32(&lineInfo->line_state,
+ ¤t_line_state,
+ LINE_LEADER_POPULATED);
About the commect
+ * started processing, no need to change the state to
+ * LINE_LEADER_POPULATING in this case.
Does it means no need to change the state to LINE_LEADER_POPULATED ' here?
3.
+ * 3) only one worker should choose one line for processing, this is handled by
+ * using pg_atomic_compare_exchange_u32, worker will change the state to
+ * LINE_WORKER_PROCESSING only if line_state is LINE_LEADER_POPULATED.
In the latest patch, it will set the state to LINE_WORKER_PROCESSING if line_state is LINE_LEADER_POPULATED or
LINE_LEADER_POPULATING.
So The comment here seems wrong.
4.
A suggestion for CacheLineInfo.
It use appendBinaryStringXXX to store the line in memory.
appendBinaryStringXXX will double the str memory when there is no enough spaces.
How about call enlargeStringInfo in advance, if we already know the whole line size?
It can avoid some memory waste and may impove a little performance.
Best regards,
houzj
