On Fri, Mar 3, 2023 at 3:02 PM Önder Kalacı <onderkal...@gmail.com> wrote:
>>
>> 7.
>> - /* Build scankey for every attribute in the index. */
>> - for (attoff = 0; attoff <
>> IndexRelationGetNumberOfKeyAttributes(idxrel); attoff++)
>> + /* Build scankey for every non-expression attribute in the index. */
>> + for (index_attoff = 0; index_attoff <
>> IndexRelationGetNumberOfKeyAttributes(idxrel);
>> + index_attoff++)
>> {
>> Oid operator;
>> Oid opfamily;
>> + Oid optype = get_opclass_input_type(opclass->values[index_attoff]);
>> RegProcedure regop;
>> - int pkattno = attoff + 1;
>> - int mainattno = indkey->values[attoff];
>> - Oid optype = get_opclass_input_type(opclass->values[attoff]);
>> + int table_attno = indkey->values[index_attoff];
>>
>> I don't think here we need to change variable names if we retain
>> mainattno as it is instead of changing it to table_attno. The current
>> naming doesn't seem bad for the current usage of the patch.
>
>
> Hmm, I'm actually not convinced that the variable naming on HEAD is good for
> the current patch. The main difference is that now we allow indexes like:
> CREATE INDEX idx ON table(foo(col), col_2)
>
> (See # Testcase start: SUBSCRIPTION CAN USE INDEXES WITH
> EXPRESSIONS AND COLUMNS)
>
> In such indexes, we could skip the attributes of the index. So, skey_attoff
> is not
> equal to index_attoff anymore. So, calling out this explicitly via the
> variable names
> seems more robust to me. Plus, mainattno sounded vague to me when I first read
> this function.
>
Yeah, I understand this part. By looking at the diff, it appeared to
me that this was an unnecessary change. Anyway, I see your point, so
if you want to keep the naming as you proposed at least don't change
the ordering for get_opclass_input_type() call because that looks odd
to me.
>
> Attached v29 for this review. Note that I'll be working on the disable index
> scan changes after
>
Okay, thanks!
--
With Regards,
Amit Kapila.
