On Wed, Jul 19, 2023 at 5:09 PM Önder Kalacı <onderkal...@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
v4-0001-Remove-unnecessary-checks-for-indexes-for-REPLICA.patch
Description: Binary data
