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

On Tue, 14 Mar 2023 at 14:36, Önder Kalacı <onderkalaci(at)gmail(dot)com> wrote:
>
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, 14 Mar 2023 Sal, 11:59 tarihinde şunu yazdı:
>>
>> On Tue, Mar 14, 2023 at 12:48 PM Önder Kalacı <onderkalaci(at)gmail(dot)com> wrote:
>> >>
>> >> 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.
>> >
>>
>> It seems you forgot to address these last two comments in the latest version.
>>
>
> Oops, sorry. I think when I get your test changes, I somehow overridden these changes
> on my local.

Thanks for the updated patch.
Few minor comments:
1) The extra line break after IsIndexOnlyOnExpression function can be removed:
+ }
+
+ return true;
+}
+
+
+/*
+ * Returns true if the attrmap (which belongs to remoterel) contains the
+ * leftmost column of the index.
+ *
+ * Otherwise returns false.
+ */

2) Generally we don't terminate with "." for single line comments
+
+ /*
+ * Simple case, we already have a primary key or a replica identity index.
+ */
+ idxoid = GetRelationIdentityOrPK(localrel);
+ if (OidIsValid(idxoid))
+ return idxoid;

Regards,
Vignesh