Re: doc: improve the restriction description of using indexes on REPLICA IDENTITY FULL table.

On Tue, Jul 11, 2023 at 1:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jul 11, 2023 at 4:54 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Mon, Jul 10, 2023 at 2:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Jul 10, 2023 at 7:55 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> > > > On Sat, Jul 8, 2023 at 1:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jul 7, 2023 at 1:36 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > > >
> > > > > > I prefer the first suggestion. I've attached the updated patch.
> > > > > >
> > > > >
> > > > > This looks mostly good to me but I think it would be better if we can
> > > > > also add the information that the leftmost index column must be a
> > > > > non-expression. So, how about: "Candidate indexes must be btree,
> > > > > non-partial, and the leftmost index column must be a non-expression
> > > > > and reference to a published table column (i.e. cannot consist of only
> > > > > expressions)."?
> > > >
> > > > That part in parentheses ought to say "the index ..." because it is
> > > > referring to the full INDEX, not to the leftmost column. I think this
> > > > was missed when Sawada-san took my previous suggestion [1].
> > > >
> > > > IMO it doesn't sound right to say the "index column must be a
> > > > non-expression". It is already a non-expression because it is a
> > > > column. So I think it would be better to refer to this as an INDEX
> > > > "field" instead of an INDEX column. Note that "field" is the same
> > > > terminology used in the docs for CREATE INDEX [2].
> > > >
> > >
> > > I thought it would be better to be explicit for this case but I am
> > > fine if Sawada-San and you prefer some other way to document it.
> > >
> >
> > I see. How about just moving the parenthesized part to explicitly
> > refer only to the leftmost field?
> >
> > SUGGESTION
> > Candidate indexes must be btree, non-partial, and the leftmost index
> > field must be a column (not an expression) that references a published
> > table column.
> >
>
> Yeah, something like that works for me.

Looks good to me. I've attached the updated patch. In the comment in
RemoteRelContainsLeftMostColumnOnIdx(), I used "remote relation"
instead of "published table" as it's more consistent with surrounding
comments. Also, I've removed the comment starting with "We also skip
indedes..." as the new comment now covers it. Please review it.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com