> +1 for the readability. I think we can as you suggest or I feel it
> would be better if we return false as soon as we found any condition
> is false. The current patch has a mixed style such that for
> InvalidStrategy, it returns immediately but for others, it does a
> combined check.
>
I have followed the later style in the attached.
Looks good to me!
I agree with your note that the confusion was mostly
around two different styles (e,g., some checks return early others wait
until the final return). Now, the code is pretty easy to follow.
> The other point we should consider in this regard is
> the below assert check:
>
> +#ifdef USE_ASSERT_CHECKING
> + {
> + /* Check that the given index access method has amgettuple routine */
> + IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(indexInfo->ii_Am,
> + false);
> + Assert(amroutine->amgettuple != NULL);
> + }
> +#endif
>
> Apart from having an assert, we have the following two options (a)
> check this condition as well and return false if am doesn't support
> amgettuple (b) report elog(ERROR, ..) in this case.
I think with the current state of the patch (e.g., we only support btree and hash),
Assert looks reasonable.
In the future, in case we have a future hypothetical index type that fulfills the
"if" checks but fails on amgettuple, we could change the code to "return false"
such that it gives a chance for the other indexes to satisfy the condition.
Let me know what you think of the attached.
Looks good to me. I have also done some testing, which works as documented/expected.
Thanks,
Onder