On Sat, Jul 15, 2023 at 2:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jul 13, 2023 at 10:55 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Jul 12, 2023 at 11:15 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > > I think this is a valid concern. Can't we move all the checks
> > > > (including the remote attrs check) inside
> > > > IsIndexUsableForReplicaIdentityFull() and then call it from both
> > > > places? Won't we have attrmap information available in the callers of
> > > > FindReplTupleInLocalRel() via ApplyExecutionData?
> > >
> > > You mean to pass ApplyExecutionData or attrmap down to
> > > RelationFindReplTupleByIndex()? I think it would be better to call it
> > > from FindReplTupleInLocalRel() instead.
> >
> > I've attached a draft patch for this idea.
> >
>
> Looks reasonable to me. However, I am not very sure if we need to
> change the prototype of RelationFindReplTupleByIndex(). Few other
> minor comments:
Agreed. I reverted the change.
>
> 1.
> - * has been implemented as a tri-state with values DISABLED, PENDING, and
> +n * has been implemented as a tri-state with values DISABLED, PENDING, and
> * ENABLED.
> *
> The above seems like a spurious change.
Fixed.
>
> 2.
> + /* And must reference the remote relation column */
> + if (attrmap->maplen <= AttrNumberGetAttrOffset(keycol) ||
> + attrmap->attnums[AttrNumberGetAttrOffset(keycol)] < 0)
> + return false;
> +
>
> I think we should specify the reason for this. I see that in the
> commit fd48a86c62 [1], the reason for this is removed. Shouldn't we
> retain that in some form?
Agreed.
I've updated the patch. Regarding one change in the patch:
* Returns the oid of an index that can be used by the apply worker to scan
- * the relation. The index must be btree or hash, non-partial, and the leftmost
- * field must be a column (not an expression) that references the remote
- * relation column. These limitations help to keep the index scan similar
- * to PK/RI index scans.
+ * the relation.
I moved the second sentence to IsIndexUsableForReplicaIdentityFull()
because this function is now responsible for checking if the given
index is usable for REPLICA IDENTITY FULL tables. I think it would be
better to mention all conditions for such usable indexes in one place.
Currently, the conditions are explained in
FindUsableIndexForReplicaIdentityFull() but the checks are performed
and the details are explained in
IsIndexUsableForReplicaIdentityFull().
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.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com