Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher - Mailing list pgsql-hackers

On Thu, Mar 9, 2023 at 5:47 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com>, 8 Mar 2023 Çar, 14:42 tarihinde şunu yazdı:
>>
>> On Wed, Mar 8, 2023 at 4:51 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>> >
>> >
>> >>
>> >> I just share this case and then we
>> >> can discuss should we pick the index which only contain the extra columns on the
>> >> subscriber.
>> >>
>> >
>> > I think its performance implications come down to the discussion on [1]. Overall, I prefer
>> > avoiding adding any additional complexity in the code for some edge cases. The code
>> > can handle this sub-optimal user pattern, with a sub-optimal performance.
>> >
>>
>> It is fine to leave this and Hou-San's case if they make the patch
>> complex. However, it may be better to give it a try and see if this or
>> other regression/optimization can be avoided without adding much
>> complexity to the patch. You can prepare a top-up patch and then we
>> can discuss it.
>>
>>
>
> Alright, I did some basic prototypes for the problems mentioned, just to show
> that these problems can be solved without too much hassle. But, the patchees
> are not complete, some tests fail, no comments / tests exist, some values should be
> cached etc.  Mostly sharing as a heads up and sharing the progress given I have not
> responded to this specific mail. I'll update these when I have some extra time after
> replying to the 0001 patch.
>

wip_for_optimize_index_column_match
+static bool
+IndexContainsAnyRemoteColumn(IndexInfo  *indexInfo,
+ LogicalRepRelation  *remoterel)
+{
+ for (int i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
+ {

Wouldn't it be better to just check if the first column is not part of
the remote column then we can skip that index?

In wip_optimize_for_non_pkey_non_ri_unique_index patch, irrespective
of whether we want to retain this set of changes, the function name
IsIdxSafeToSkipDuplicates() sounds better than
IdxIsRelationIdentityOrPK() and we can even change the check to
GetRelationIdentityOrPK() instead of separate checks replica index and
PK. So, it would be better to include this part of the change (a.
change the function name to IsIdxSafeToSkipDuplicates() and (b) change
the check to use GetRelationIdentityOrPK()) in the main patch.

--
With Regards,
Amit Kapila.