Re: POC, WIP: OR-clause support for indexes

Em seg., 10 de jul. de 2023 às 09:03, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
escreveu:

> Hi Alena,
>
> Em seg., 10 de jul. de 2023 às 05:38, Alena Rybakina <
> lena(dot)ribackina(at)yandex(dot)ru> escreveu:
>
>> I agreed with the changes. Thank you for your work.
>>
>> I updated patch and added you to the authors.
>>
>> I specified Ranier Vilela as a reviewer.
>>
> Is a good habit when post a new version of the patch, name it v1, v2,
> v3,etc.
> Makes it easy to follow development and references on the thread.
>
> Regarding the last patch.
> 1. I think that variable const_is_left is not necessary.
> You can stick with:
> + if (IsA(get_leftop(orqual), Const))
> + nconst_expr =get_rightop(orqual);
> + const_expr = get_leftop(orqual) ;
> + else if (IsA(get_rightop(orqual), Const))
> + nconst_expr =get_leftop(orqual);
> + const_expr = get_rightop(orqual) ;
> + else
> + {
> + or_list = lappend(or_list, orqual);
> + continue;
> + }
>
> 2. Test scalar_type != RECORDOID is more cheaper,
> mainly if OidIsValid were a function, we knows that is a macro.
> + if (scalar_type != RECORDOID && OidIsValid(scalar_type))
>
> 3. Sorry about wrong tip about array_type, but if really necessary,
> better use it.
> + newa->element_typeid = scalar_type;
> + newa->array_typeid = array_type;
>
> 4. Is a good habit, call free last, to avoid somebody accidentally using
> it.
> + or_list = lappend(or_list, gentry->expr);
> + list_free(gentry->consts);
> + continue;
>
> 5. list_make1(makeString((char *) "=")
> Is an invariant?
>
Please nevermind 5. Is not invariant.

regards,
Ranier Vilela