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.
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 ...
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);
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 :)
Thanks,
Onder