On Wed, Jul 19, 2023 at 5:09 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> Hi Masahiko, Amit, all
>
>> I've updated the patch.
>
>
> I think the flow is much nicer now compared to the HEAD. I really don't have any
> comments regarding the accuracy of the code changes, all looks good to me.
> Overall, I cannot see any behavioral changes as you already alluded to.
Thank you for reviewing the patch.
>
> Maybe few minor notes regarding the comments:
>
>> /*
>> + * And must reference the remote relation column. This is because if it
>> + * doesn't, the sequential scan is favorable over index scan in most
>> + * cases..
>> + */
>
>
> I think the reader might have lost the context (or say in the future due to
> another refactor etc). So maybe start with:
>
>> /* And the leftmost index field must refer to the ...
Fixed.
>
>
> Also, now in IsIndexUsableForReplicaIdentityFull() some of the conditions have comments
> some don't. Should we comment on the missing ones as well, maybe such as:
>
>> /* partial indexes are not support *
>> if (indexInfo->ii_Predicate != NIL)
>
> and,
>>
>> /* all indexes must have at least one attribute */
>> Assert(indexInfo->ii_NumIndexAttrs >= 1);
Agreed. But I don't think the latter comment is necessary as it's obvious.
>
>
>
>>
>>>
>>>
>>> BTW, IsIndexOnlyExpression() is not necessary but the current code
>>> still works fine. So do we need to backpatch it to PG16? I'm thinking
>>> we can apply it to only HEAD.
>>
>> Either way is fine but I think if we backpatch it then the code
>> remains consistent and the backpatching would be easier.
>
>
> Yeah, I also have a slight preference for backporting. It could make it easier to maintain the code
> in the future in case of another backport(s). With the cost of making it slightly harder for you now :)
Agreed.
I've attached the updated patch. I'll push it early next week, barring
any objections.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com