Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

Hi Shi Yu,

> in RemoteRelContainsLeftMostColumnOnIdx():
>
> + if (indexInfo->ii_NumIndexAttrs < 1)
> + return false;
>
> Did you see any cases that the condition is true? I think there is at
> least one
> column in the index. If so, we can use an Assert().
>

Actually, it was mostly to guard against any edge cases. I thought similar
to tables,
we can have zero column indexes, but it turns out it is not possible. Also,
index_create seems to check that, so changing it asset makes sense:

>
> /*
> * check parameters
> */
> if (indexInfo->ii_NumIndexAttrs < 1)
> elog(ERROR, "must index at least one column");

>
> + if (attrmap->maplen <= AttrNumberGetAttrOffset(keycol))
> + return false;
>
> Similarly, I think `attrmap->maplen` is the number of columns and it is
> always
> greater than keycol. If you agree, we can check it with an Assert().

At this point, I'm really hesitant to make any assumptions. Logical
replication
is pretty flexible, and who knows maybe dropped columns will be treated
differently at some point, and this assumption changes?

I really feel more comfortable to keep this as-is. We call this function
very infrequently
anyway.

> Besides, It
> seems we don't need AttrNumberGetAttrOffset().
>
>
Why not? We are accessing the AttrNumberGetAttrOffset(keycol) element
of the array attnums?

> 2.
> +# make sure that the subscriber has the correct data after the update
> UPDATE
>
> "update UPDATE" seems to be a typo.
>
>
thanks, fixed

> 3.
> +# now, drop the index with the expression, and re-create index on column
> lastname
>
> The comment says "re-create index on column lastname" but it seems we
> didn't do
> that, should it be modified to something like:
> # now, drop the index with the expression, we will use sequential scan
>
>
>
Thanks, fixed

I'll add the changes to v49 in the next e-mail.

Thanks,
Onder KALACI