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

From: Amit Kapila <[email-masked]>
Date: Sat, 4 Mar 2023 16:03:53 +0530

On Fri, Mar 3, 2023 at 1:09 PM Önder Kalacı <[email-masked]> wrote:
>
>>
>> 5.
>>
>> +       /*
>> +        * If index scans are disabled, use a sequential scan.
>> +        *
>> +        * Note that we do not use index scans below when enable_indexscan is
>> +        * false. Allowing primary key or replica identity even when index scan is
>> +        * disabled is the legacy behaviour. So we hesitate to move the below
>> +        * enable_indexscan check to be done earlier in this function.
>> +        */
>> +       if (!enable_indexscan)
>> +               return InvalidOid;
>>
>> Since the document of enable_indexscan says "Enables or disables the query
>> planner's use of index-scan plan types. The default is on.", and we don't use
>> planner here, so I am not sure should we allow/disallow index scan in apply
>> worker based on this GUC.
>>
>
> Given Amit's suggestion on [1], I'm planning to drop this check altogether, and
> rely on table storage parameters.
>

This still seems to be present in the latest version. I think we can
just remove this and then add the additional check as suggested by you
as part of the second patch.

Few other comments on latest version:
==============================
1.
+/*
+ * Returns true if the index is usable for replica identity full. For details,
+ * see FindUsableIndexForReplicaIdentityFull.
+ */
+bool
+IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo)
+{
+ bool is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
+ bool is_partial = (indexInfo->ii_Predicate != NIL);
+ bool is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+
+ if (is_btree && !is_partial && !is_only_on_expression)
+ {
+ return true;
...
...
+/*
+ * Returns the oid of an index that can be used via the apply worker. The index
+ * should be btree, non-partial and have at least one column reference (e.g.,
+ * should not consist of only expressions). The limitations arise from
+ * RelationFindReplTupleByIndex(), which is designed to handle PK/RI and these
+ * limitations are inherent to PK/RI.

By these two, the patch infers that it picks an index that adheres to
the limitations of PK/RI. Apart from unique, the other properties of
RI are "not partial, not deferrable, and include only columns marked
NOT NULL". See ATExecReplicaIdentity() for corresponding checks. We
don't try to ensure the last two from the list. It is fine to do so if
we document the reasons for the same in comments or we can even try to
enforce the remaining restrictions as well. For example, it should be
okay to allow NULL column values because we anyway compare the entire
tuple after getting the value from the index.

2.
+ {
+ /*
+ * This attribute is an expression, and
+ * FindUsableIndexForReplicaIdentityFull() was called earlier
+ * when the index for subscriber was selected. There, the indexes
+ * comprising *only* expressions have already been eliminated.
+ *
+ * Also, because PK/RI can't include expressions we
+ * sanity check the index is neither of those kinds.
+ */
+ Assert(!IdxIsRelationIdentityOrPK(rel, idxrel->rd_id));

This comment doesn't make much sense after you have moved the
corresponding Assert in RelationFindReplTupleByIndex(). Either we
should move or remove this Assert as well or at least update the
comments to reflect the latest code.

3. When FindLogicalRepUsableIndex() is invoked from
logicalrep_partition_open(), the current memory context would be
LogicalRepPartMapContext which would be a long-lived context and we
allocate memory for indexes in FindLogicalRepUsableIndex() which can
accumulate over a period of time. So, I think it would be better to
switch to the old context in logicalrep_partition_open() before
invoking FindLogicalRepUsableIndex() provided that is not a long-lived
context.

-- 
With Regards,
Amit Kapila.

---

From: Önder Kalacı <[email-masked]>
Date: Mon, 6 Mar 2023 12:08:20 +0300

Hi Amit, all

> as part of the second patch.
Now attaching the second patch as well, which implements a new
storage parameter as
you suggested earlier.

I'm open for naming suggestions, I wanted to make the name explicit, so it
is a little long.

I'm also not very familiar with the sgml format. I mostly followed the
existing docs and
built the docs for inspection, but it would be good to have a look into
that part
a little bit further in case there I missed something important etc.

> Few other comments on latest version:

I think this is a documentation issue of this patch. I improved the wording
a bit
more. Does that look better?

I also went over the code / docs to see if we have
any other such documentation issues, I also
updated logical-replication.sgml.

I'd prefer to support NULL values as there is no harm in doing that and it
is
pretty useful feature (we also have tests covering it).

To my knowledge, I don't see any problems with deferrable as we are only
interested in the indexes, not with the constraint. Still, if you see any,
I can
add the check for that. (Here, the user could still have unique index that
is associated with a constraint, but still, I don't see any edge cases
regarding deferrable constraints).

> comments to reflect the latest code.

I think removing that Assert is fine after having a more generic
Assert in RelationFindReplTupleByIndex().

I mostly left that comment so that the meaning of
AttributeNumberIsValid() is easier for readers to follow. But, now
I'm also leaning towards removing the comment and Assert.

> invoking FindLogicalRepUsableIndex() provided that is not a long-lived
Hmm, makes sense, that'd avoid any potential leaks that this patch
might bring. Applied your suggestion. Also, looking at the same function
call in logicalrep_rel_open(), that already seems safe regarding leaks. Do
you see any problems with that?

Attached v32. I'll continue replying to the e-mails on this thread with
different
patches. I'm assuming this is easier for you to review such that we have
different
patches for each review. If not, please let me know, I can reply to all
mails
at once.

Thanks,
Onder KALACI

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 6 Mar 2023 16:18:10 +0530

On Mon, Mar 6, 2023 at 2:38 PM Önder Kalacı <[email-masked]> wrote:
I was going through the thread and patch,  I noticed that in the
initial version, we were depending upon the planner to let it decide
whether index scan is cheaper or not and which index to pick.  But in
the latest patch if a useful index exists then we chose that without
comparing the cost of whether it is cheaper than sequential scan or
not.  Is my understanding correct?  What is the reason for the same,
one reason I could see while looking into the thread is that we can
not just decide once whether the index scan is cheaper or not because
that decision could change in the future but isn't that better than
never checking whether index scan is cheaper or not.  Because in some
cases where column selectivity is high like 80-90% then the index can
be very costly due to random page fetches.  So I think we could easily
produce regression in some cases, have we tested those cases?

Let me if I am missing something.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

---

From: Amit Kapila <[email-masked]>
Date: Mon, 6 Mar 2023 16:44:59 +0530

> not.  Is my understanding correct?  What is the reason for the same,

Yes, your understanding is correct. The main reason is that we don't
have an agreement on using the internal planner APIs for apply. That
will be a long-term maintenance burden. See discussion around email
[1]. So, we decided to use the current infrastructure to achieve index
scans during apply when publisher has replica identity full. This will
still be win in many cases and we are planning to provide a knob to
disable this feature.

[1] - https://www.postgresql.org/message-id/3466340.1673117404%40sss.pgh.pa.us

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 6 Mar 2023 19:25:37 +0530

> [1] - https://www.postgresql.org/message-id/3466340.1673117404%40sss.pgh.pa.us

Okay, this makes sense, so basically, in "replica identify full" case
instead of doing the default sequence scan we will provide a knob to
either choose index scan or sequence scan, and that seems reasonable
to me.