Thanks for having a look at this.
On Tue, 26 Jan 2021 at 15:48, Zhihong Yu <zyu@yugabyte.com> wrote:
> bq. within this range. Table AMs where scanning ranges of TIDs does not make
> sense or is difficult to implement efficiently may choose to not implement
>
> Is there criterion on how to judge efficiency ?
For example, if the AM had no way to index such a column and the
method needed to scan the entire table to find TIDs in that range. The
planner may as well just pick a SeqScan. If that's the case, then the
table AM may as well not bother implementing that function.
> + if (tidopexpr->exprtype == TIDEXPR_LOWER_BOUND)
> ...
> + if (tidopexpr->exprtype == TIDEXPR_UPPER_BOUND)
>
> The if statement for upper bound should be prefixed with 'else', right ?
Yeah, thanks.
> + * TidRecheck -- access method routine to recheck a tuple in EvalPlanQual
> ...
> +TidRangeRecheck(TidRangeScanState *node, TupleTableSlot *slot)
>
> The method name in the comment doesn't match the real method name.
Well noticed.
> + * ExecTidRangeScan(node)
> ...
> +ExecTidRangeScan(PlanState *pstate)
>
> Parameter names don't match.
hmm. Looking around it seems there's lots of other places that do
this. I think the the comment is really just indicating that the
function is taking an executor node state as a parameter.
Have a look at: git grep -E "\s\*.*\(node\)$" that shows the other
places. Some of these have the parameter named "node", and many others
have some other name.
I've made the two changes locally. Since the two issues were mostly
cosmetic, I'll post an updated patch after some bigger changes are
required.
Thanks again for looking at this.
David
