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

On Tue, Jul 12, 2022 at 7:07 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> Hi hackers,
>
>
> It is often not feasible to use `REPLICA IDENTITY FULL` on the publication, because it leads to full table scan
>
> per tuple change on the subscription. This makes `REPLICA IDENTITY FULL` impracticable -- probably other
>
> than some small number of use cases.
>

IIUC, this proposal is to optimize cases where users can't have a
unique/primary key for a relation on the subscriber and those
relations receive lots of updates or deletes?

> With this patch, I'm proposing the following change: If there is an index on the subscriber, use the index
>
> as long as the planner sub-modules picks any index over sequential scan.
>
> Majority of the logic on the subscriber side has already existed in the code. The subscriber is already
>
> capable of doing (unique) index scans. With this patch, we are allowing the index to iterate over the
>
> tuples fetched and only act when tuples are equal. The ones familiar with this part of the code could
>
> realize that the sequential scan code on the subscriber already implements the `tuples_equal()` function.
>
> In short, the changes on the subscriber are mostly combining parts of (unique) index scan and
>
> sequential scan codes.
>
> The decision on whether to use an index (or which index) is mostly derived from planner infrastructure.
>
> The idea is that on the subscriber we have all the columns. So, construct all the `Path`s with the
>
> restrictions on all columns, such as `col_1 = $1 AND col_2 = $2 ... AND col_n = $N`. Finally, let
>
> the planner sub-module -- `create_index_paths()` -- to give us the relevant  index `Path`s. On top of
>
> that adds the sequential scan `Path` as well. Finally, pick the cheapest `Path` among.
>
> From the performance point of view, there are few things to note. First, the patch aims not to
> change the behavior when PRIMARY KEY or UNIQUE INDEX is used. Second, when REPLICA IDENTITY
> IS FULL on the publisher and an index is used on the subscriber, the difference mostly comes down
> to `index scan` vs `sequential scan`. That's why it is hard to claim a certain number of improvements.
> It mostly depends on the data size, index and the data distribution.
>

It seems that in favorable cases it will improve performance but we
should consider unfavorable cases as well. Two things that come to
mind in that regard are (a) while choosing index/seq. scan paths, the
patch doesn't account for cost for tuples_equal() which needs to be
performed for index scans, (b) it appears to me that the patch decides
which index to use the first time it opens the rel (or if the rel gets
invalidated) on subscriber and then for all consecutive operations it
uses the same index. It is quite possible that after some more
operations on the table, using the same index will actually be
costlier than a sequence scan or some other index scan.

--
With Regards,
Amit Kapila.

On Mon, Jul 18, 2022 at 11:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jul 12, 2022 at 7:07 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> >
> > Hi hackers,
> >
> >
> > It is often not feasible to use `REPLICA IDENTITY FULL` on the publication, because it leads to full table scan
> >
> > per tuple change on the subscription. This makes `REPLICA IDENTITY FULL` impracticable -- probably other
> >
> > than some small number of use cases.
> >
>
> IIUC, this proposal is to optimize cases where users can't have a
> unique/primary key for a relation on the subscriber and those
> relations receive lots of updates or deletes?
>
> > With this patch, I'm proposing the following change: If there is an index on the subscriber, use the index
> >
> > as long as the planner sub-modules picks any index over sequential scan.
> >
> > Majority of the logic on the subscriber side has already existed in the code. The subscriber is already
> >
> > capable of doing (unique) index scans. With this patch, we are allowing the index to iterate over the
> >
> > tuples fetched and only act when tuples are equal. The ones familiar with this part of the code could
> >
> > realize that the sequential scan code on the subscriber already implements the `tuples_equal()` function.
> >
> > In short, the changes on the subscriber are mostly combining parts of (unique) index scan and
> >
> > sequential scan codes.
> >
> > The decision on whether to use an index (or which index) is mostly derived from planner infrastructure.
> >
> > The idea is that on the subscriber we have all the columns. So, construct all the `Path`s with the
> >
> > restrictions on all columns, such as `col_1 = $1 AND col_2 = $2 ... AND col_n = $N`. Finally, let
> >
> > the planner sub-module -- `create_index_paths()` -- to give us the relevant  index `Path`s. On top of
> >
> > that adds the sequential scan `Path` as well. Finally, pick the cheapest `Path` among.
> >
> > From the performance point of view, there are few things to note. First, the patch aims not to
> > change the behavior when PRIMARY KEY or UNIQUE INDEX is used. Second, when REPLICA IDENTITY
> > IS FULL on the publisher and an index is used on the subscriber, the difference mostly comes down
> > to `index scan` vs `sequential scan`. That's why it is hard to claim a certain number of improvements.
> > It mostly depends on the data size, index and the data distribution.
> >
>
> It seems that in favorable cases it will improve performance but we
> should consider unfavorable cases as well. Two things that come to
> mind in that regard are (a) while choosing index/seq. scan paths, the
> patch doesn't account for cost for tuples_equal() which needs to be
> performed for index scans, (b) it appears to me that the patch decides
> which index to use the first time it opens the rel (or if the rel gets
> invalidated) on subscriber and then for all consecutive operations it
> uses the same index. It is quite possible that after some more
> operations on the table, using the same index will actually be
> costlier than a sequence scan or some other index scan.
>

Point (a) won't matter because we perform tuples_equal both for
sequence and index scans. So, we can ignore point (a).

--
With Regards,
Amit Kapila.

On Tue, Jul 19, 2022 at 1:46 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> Hi, thanks for your reply.
>
> Amit Kapila <amit.kapila16@gmail.com>, 18 Tem 2022 Pzt, 08:29 tarihinde şunu yazdı:
>>
>> >
>>
>> IIUC, this proposal is to optimize cases where users can't have a
>> unique/primary key for a relation on the subscriber and those
>> relations receive lots of updates or deletes?
>
>
> Yes, that is right.
>
> In a similar perspective, I see this patch useful for reducing the "use primary key/unique index" requirement to "use
anyindex" for a reasonably performant logical replication with updates/deletes. 
>

Agreed. BTW, have you seen any such requirements from users where this
will be useful for them?

>>
>>
>> It seems that in favorable cases it will improve performance but we
>> should consider unfavorable cases as well. Two things that come to
>> mind in that regard are (a) while choosing index/seq. scan paths, the
>> patch doesn't account for cost for tuples_equal() which needs to be
>> performed for index scans, (b) it appears to me that the patch decides
>> which index to use the first time it opens the rel (or if the rel gets
>> invalidated) on subscriber and then for all consecutive operations it
>> uses the same index. It is quite possible that after some more
>> operations on the table, using the same index will actually be
>> costlier than a sequence scan or some other index scan
>
>
> Regarding (b), yes that is a concern I share. And, I was actually considering sending another patch regarding this.
>
> Currently, I can see two options and happy to hear your take on these (or maybe another idea?)
>
> - Add a new class of invalidation callbacks: Today, if we do ALTER TABLE or CREATE INDEX on a table, the
CacheRegisterRelcacheCallbackhelps us to re-create the cache entries. In this case, as far as I can see, we need a
callbackthat is called when table "ANALYZE"d, because that is when the statistics change. That is the time picking a
newindex makes sense. 
> However, that seems like adding another dimension to this patch, which I can try but also see that committing becomes
evenharder. 
>

This idea sounds worth investigating. I see that this will require
more work but OTOH, we can't allow the existing system to regress
especially because depending on workload it might regress badly. We
can create a patch for this atop the base patch for easier review/test
but I feel we need some way to address this point.

 So, please see the next idea as well.
>
> - Ask users to manually pick the index they want to use: Currently, the main complexity of the patch comes with the
plannerrelated code. In fact, if you look into the logical replication related changes, those are relatively modest
changes.If we can drop the feature that Postgres picks the index, and provide a user interface to set the indexes per
tablein the subscription, we can probably have an easier patch to review & test. For example, we could add `ALTER
SUBSCRIPTIONsub ALTER TABLE t USE INDEX i` type of an API. This also needs some coding, but probably much simpler than
thecurrent code. And, obviously, this pops up the question of can users pick the right index? 
>

I think picking the right index is one point and another is what if
the subscription has many tables (say 10K or more), doing it for
individual tables per subscription won't be fun. Also, users need to
identify which tables belong to a particular subscription, now, users
can find the same via pg_subscription_rel or some other way but doing
this won't be straightforward for users. So, my inclination would be
to pick the right index automatically rather than getting the input
from the user.

Now, your point related to planner code in the patch bothers me as
well but I haven't studied the patch in detail to provide any
alternatives at this stage. Do you have any other ideas to make it
simpler or solve this problem in some other way?

--
With Regards,
Amit Kapila.

On Wed, Jul 20, 2022 at 8:19 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
>>
>> > - Add a new class of invalidation callbacks: Today, if we do ALTER TABLE or CREATE INDEX on a table, the
CacheRegisterRelcacheCallbackhelps us to re-create the cache entries. In this case, as far as I can see, we need a
callbackthat is called when table "ANALYZE"d, because that is when the statistics change. That is the time picking a
newindex makes sense. 
>> > However, that seems like adding another dimension to this patch, which I can try but also see that committing
becomeseven harder. 
>> >
>>
>> This idea sounds worth investigating. I see that this will require
>> more work but OTOH, we can't allow the existing system to regress
>> especially because depending on workload it might regress badly.
>
>
> Just to note if that is not clear: This patch avoids (or at least aims to avoid assuming no bugs) changing the
behaviorof the existing systems with PRIMARY KEY or UNIQUE index. In that case, we still use the relevant indexes. 
>
>>
>> We
>> can create a patch for this atop the base patch for easier review/test
>> but I feel we need some way to address this point.
>>
>
> One another idea could be to re-calculate the index, say after N updates/deletes for the table. We may consider using
subscription_parameterfor getting N -- with a good default, or even hard-code into the code. I think the cost of
re-calculatingshould really be pretty small compared to the other things happening during logical replication. So, a
sanedefault might work? 
>

One difficulty in deciding the value of N for the user or choosing a
default would be that we need to probably consider the local DML
operations on the table as well.

> If you think the above doesn't work, I can try to work on a separate patch which adds something like "analyze
invalidationcallback". 
>

I suggest we should give this a try and if this turns out to be
problematic or complex then we can think of using some heuristic as
you are suggesting above.

>
>>
>>
>> Now, your point related to planner code in the patch bothers me as
>> well but I haven't studied the patch in detail to provide any
>> alternatives at this stage. Do you have any other ideas to make it
>> simpler or solve this problem in some other way?
>>
>
> One idea I tried earlier was to go over the existing indexes and on the table, then get the IndexInfo via
BuildIndexInfo().And then, try to find a good heuristic to pick an index. In the end, I felt like that is doing a
sub-optimaljob, requiring a similar amount of code of the current patch, and still using the similar infrastructure. 
>
> My conclusion for that route was I should either use a very simple heuristic (like pick the index with the most
columns)and have a suboptimal index pick, 
>

Not only that but say all index have same number of columns then we
need to probably either pick the first such index or use some other
heuristic.

>
> OR use a complex heuristic with a reasonable index pick. And, the latter approach converged to the planner code in
thepatch. Do you think the former approach is acceptable? 
>

In this regard, I was thinking in which cases a sequence scan can be
better than the index scan (considering one is available). I think if
a certain column has a lot of duplicates (for example, a column has a
boolean value) then probably doing a sequence scan is better. Now,
considering this even though your other approach sounds simpler but
could lead to unpredictable results. So, I think the latter approach
is preferable.

BTW, do we want to consider partial indexes for the scan in this
context? I mean it may not have data of all rows so how that would be
usable?

Few comments:
===============
1.
static List *
+CreateReplicaIdentityFullPaths(Relation localrel)
{
...
+ /*
+ * Rather than doing all the pushups that would be needed to use
+ * set_baserel_size_estimates, just do a quick hack for rows and width.
+ */
+ rel->rows = rel->tuples;

Is it a good idea to set rows without any selectivity estimation?
Won't this always set the entire rows in a relation? Also, if we don't
want to use set_baserel_size_estimates(), how will we compute
baserestrictcost which will later be used in the costing of paths (for
example, costing of seqscan path (cost_seqscan) uses it)?

In general, I think it will be better to consider calling some
top-level planner functions even for paths. Can we consider using
make_one_rel() instead of building individual paths? On similar lines,
in function PickCheapestIndexPathIfExists(), can we use
set_cheapest()?

2.
@@ -57,9 +60,6 @@ build_replindex_scan_key(ScanKey skey, Relation rel,
Relation idxrel,
  int2vector *indkey = &idxrel->rd_index->indkey;
  bool hasnulls = false;

- Assert(RelationGetReplicaIndex(rel) == RelationGetRelid(idxrel) ||
-    RelationGetPrimaryKeyIndex(rel) == RelationGetRelid(idxrel));

You have removed this assertion but there is a comment ("This is not
generic routine, it expects the idxrel to be replication identity of a
rel and meet all limitations associated with that.") atop this
function which either needs to be changed/removed and probably we
should think if the function needs some change after removing that
restriction.

--
With Regards,
Amit Kapila.

On Mon, Aug 1, 2022 at 9:52 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> Hi,
>
> As far as I can see, the following is the answer to the only remaining open discussion in this thread. Let me know if
anythingis missed. 
>
>> (b) it appears to me that the patch decides
>> >> which index to use the first time it opens the rel (or if the rel gets
>> >> invalidated) on subscriber and then for all consecutive operations it
>> >> uses the same index. It is quite possible that after some more
>> >> operations on the table, using the same index will actually be
>> >> costlier than a sequence scan or some other index scan
>> >
>> >
>> > Regarding (b), yes that is a concern I share. And, I was actually considering sending another patch regarding
this.
>> >
>> > Currently, I can see two options and happy to hear your take on these (or maybe another idea?)
>> >
>> > - Add a new class of invalidation callbacks: Today, if we do ALTER TABLE or CREATE INDEX on a table, the
CacheRegisterRelcacheCallbackhelps us to re-create the cache entries. In this case, as far as I can see, we need a
callbackthat is called when table "ANALYZE"d, because that is when the statistics change. That is the time picking a
newindex makes sense. 
>> > However, that seems like adding another dimension to this patch, which I can try but also see that committing
becomeseven harder. 
>> >
>>
>> This idea sounds worth investigating. I see that this will require
>> more work but OTOH, we can't allow the existing system to regress
>> especially because depending on workload it might regress badly. We
>> can create a patch for this atop the base patch for easier review/test
>> but I feel we need some way to address this point.
>>
>
> It turns out that we already invalidate the relevant entries in LogicalRepRelMap/LogicalRepPartMap when "ANALYZE" (or
VACUUM)updates any of the statistics in pg_class. 
>
> The call-stack for analyze is roughly:
> do_analyze_rel()
>    -> vac_update_relstats()
>      -> heap_inplace_update()
>          -> if needs to apply any statistical change
>              -> CacheInvalidateHeapTuple()
>

Yeah, it appears that this will work but I see that we don't update
here for inherited stats, how does it work for such cases?

--
With Regards,
Amit Kapila.

On Fri, Jul 22, 2022 at 9:45 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>>
>>
>> BTW, do we want to consider partial indexes for the scan in this
>> context? I mean it may not have data of all rows so how that would be
>> usable?
>>
>
> As far as I can see, check_index_predicates() never picks a partial index for the baserestrictinfos we create in
CreateReplicaIdentityFullPaths().The reason is that we have roughly the following call stack: 
>
> -check_index_predicates
>  --predicate_implied_by
> ---predicate_implied_by_recurse
> ----predicate_implied_by_simple_clause
> -----operator_predicate_proof
>
> And, inside operator_predicate_proof(), there is never going to be an equality. Because, we push `Param`s to the
baserestrictinfoswhereas the index predicates are always `Const`. 
>

I agree that the way currently baserestrictinfos are formed by patch,
it won't select the partial path, and chances are that that will be
true in future as well but I think it is better to be explicit in this
case to avoid creating a dependency between two code paths.

Few other comments:
==================
1. Why is it a good idea to choose the index selected even for the
bitmap path (T_BitmapHeapScan or T_BitmapIndexScan)? We use index scan
during update/delete, so not sure how we can conclude to use index for
bitmap paths.

2. The index info is built even on insert, so workload, where there
are no updates/deletes or those are not published then this index
selection work will go waste. Will it be better to do it at first
update/delete? One can say that it is not worth the hassle as anyway
it will be built the first time we perform an operation on the
relation or after the relation gets invalidated. If we think so, then
probably adding a comment could be useful.

3.
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT
IN ('r', 's');";
...
...
+# wait for initial table synchronization to finish
+$node_subscriber->poll_query_until('postgres', $synced_query)
+  or die "Timed out while waiting for subscriber to synchronize data";

You can avoid such instances in the test by using the new
infrastructure added in commit 0c20dd33db.

4.
  LogicalRepRelation *remoterel = &root->remoterel;
+
  Oid partOid = RelationGetRelid(partrel);

Spurious line addition.

--
With Regards,
Amit Kapila.

On Tuesday, August 9, 2022 12:59 AM Önder Kalacı <onderkalaci@gmail.com> wrote:
> Attaching v5 of the patch which reflects the review on this email, also few
> minor test improvements.
Hi,


Thank you for the updated patch.
FYI, I noticed that v5 causes cfbot failure in [1].
Could you please fix it in the next version ?


[19:44:38.420] execReplication.c: In function ‘RelationFindReplTupleByIndex’:
[19:44:38.420] execReplication.c:186:24: error: ‘eq’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
[19:44:38.420]   186 |   if (!indisunique && !tuples_equal(outslot, searchslot, eq))
[19:44:38.420]       |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[19:44:38.420] cc1: all warnings being treated as errors



[1] - https://cirrus-ci.com/task/6544573026533376



Best Regards,
    Takamichi Osumi

On Mon, Aug 8, 2022 at 9:29 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> Hi,
>
> Thanks for the feedback, see my reply below.
>
>> >
>> > It turns out that we already invalidate the relevant entries in LogicalRepRelMap/LogicalRepPartMap when "ANALYZE"
(orVACUUM) updates any of the statistics in pg_class. 
>> >
>> > The call-stack for analyze is roughly:
>> > do_analyze_rel()
>> >    -> vac_update_relstats()
>> >      -> heap_inplace_update()
>> >          -> if needs to apply any statistical change
>> >              -> CacheInvalidateHeapTuple()
>> >
>>
>> Yeah, it appears that this will work but I see that we don't update
>> here for inherited stats, how does it work for such cases?
>
>
> There, the expansion of the relation list to partitions happens one level above on the call stack. So, the call stack
lookslike the following: 
>
> autovacuum_do_vac_analyze() (or ExecVacuum)
>   -> vacuum()
>      -> expand_vacuum_rel()
>           -> rel_list=parent+children partitions
>       -> for rel in rel_list
>           ->analyze_rel()
>               ->do_analyze_rel
>                   ... (and the same call stack as above)
>
> I also added one variation of a similar test for partitioned tables, which I earlier added for non-partitioned tables
aswell: 
>
>> Added a test which triggers this behavior. The test is as follows:
>> - Create two indexes on the target, on column_a and column_b
>> - Initially load data such that the column_a has a high cardinality
>> - Show that we use the index on column_a on a child table
>> - Load more data such that the column_b has higher cardinality
>> - ANALYZE on the parent table
>> - Show that we use the index on column_b afterwards on the child table
>
>
> My answer for the above assumes that your question is regarding what happens if you ANALYZE on a partitioned table.
Ifyour question is something different, please let me know. 
>

I was talking about inheritance cases, something like:
create table tbl1 (a int);
create table tbl1_part1 (b int) inherits (tbl1);
create table tbl1_part2 (c int) inherits (tbl1);

What we do in such cases is documented as: "if the table being
analyzed has inheritance children, ANALYZE gathers two sets of
statistics: one on the rows of the parent table only, and a second
including rows of both the parent table and all of its children. This
second set of statistics is needed when planning queries that process
the inheritance tree as a whole. The child tables themselves are not
individually analyzed in this case."

Now, the point I was worried about was what if the changes in child
tables (*_part1, *_part2) are much more than in tbl1? In such cases,
we may not invalidate child rel entries, so how will logical
replication behave for updates/deletes on child tables? There may not
be any problem here but it is better to do some analysis of such cases
to see how it behaves.

>>
>> >> BTW, do we want to consider partial indexes for the scan in this
>> >> context? I mean it may not have data of all rows so how that would be
>> >> usable?
>> >>
>> >
>
>> Few other comments:
>> ==================
>> 1. Why is it a good idea to choose the index selected even for the
>> bitmap path (T_BitmapHeapScan or T_BitmapIndexScan)? We use index scan
>> during update/delete, so not sure how we can conclude to use index for
>> bitmap paths.
>
>
> In our case, during update/delete we are searching for a single tuple on the target. And, it seems like using an
indexis probably going to be cheaper for finding the single tuple. In general, I thought we should use an index if the
plannerever decides to use it with the given restrictions. 
>

What about the case where the index has a lot of duplicate values? We
may need to retrieve multiple tuples in such cases.

> Also, for the majority of the use-cases, I think we'd probably expect an index on a column with high cardinality --
henceuse index scan. So, bitmap index scans are probably not going to be that much common. 
>

You are probably right here but I don't think we can make such
assumptions. I think the safest way to avoid any regression here is to
choose an index when the planner selects an index scan. We can always
extend it later to bitmap scans if required. We can add a comment
indicating the same.

> Still, I don't see a problem with using such indexes. Of course, it is possible that I might be missing something. Do
youhave any specific concerns in this area? 
>
>>
>>
>> 2. The index info is built even on insert, so workload, where there
>> are no updates/deletes or those are not published then this index
>> selection work will go waste. Will it be better to do it at first
>> update/delete? One can say that it is not worth the hassle as anyway
>> it will be built the first time we perform an operation on the
>> relation or after the relation gets invalidated.
>
>
> With the current approach, the index (re)-calculation is coupled with (in)validation of the relevant cache entries.
So,I'd argue for the simplicity of the code, we could afford to waste this small overhead? According to my local
measurements,especially for large tables, the index oid calculation is mostly insignificant compared to the rest of the
steps.Does that sound OK to you? 
>
>
>>
>> If we think so, then
>> probably adding a comment could be useful.
>
>
> Yes, that is useful if you are OK with the above, added.
>

*
+ /*
+ * For insert-only workloads, calculating the index is not necessary.
+ * As the calculation is not expensive, we are fine to do here (instead
+ * of during first update/delete processing).
+ */

I think here instead of talking about cost, we should mention that it
is quite an infrequent operation i.e performed only when we first time
performs an operation on the relation or after invalidation. This is
because I think the cost is relative.

*
+
+ /*
+ * Although currently it is not possible for planner to pick a
+ * partial index or indexes only on expressions,

It may be better to expand this comment by describing a bit why it is
not possible in our case. You might want to give the function
reference where it is decided.

--
With Regards,
Amit Kapila.

On Sat, Aug 20, 2022 7:02 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> Hi,
> 
> I'm a little late to catch up with your comments, but here are my replies:

Thanks for your patch. Here are some comments.

1.
In FilterOutNotSuitablePathsForReplIdentFull(), is "nonPartialIndexPathList" a
good name for the list? Indexes on only expressions are also be filtered.

+static List *
+FilterOutNotSuitablePathsForReplIdentFull(List *pathlist)
+{
+    ListCell   *lc;
+    List *nonPartialIndexPathList = NIL;

2.
+typedef struct LogicalRepPartMapEntry
+{
+    Oid            partoid;        /* LogicalRepPartMap's key */
+    LogicalRepRelMapEntry relmapentry;
+    Oid            usableIndexOid; /* which index to use? (Invalid when no index
+                                 * used) */
+} LogicalRepPartMapEntry;

For partition tables, is it possible to use relmapentry->usableIndexOid to mark
which index to use? Which means we don't need to add "usableIndexOid" to
LogicalRepPartMapEntry.

3.
It looks we should change the comment for FindReplTupleInLocalRel() in this
patch.

/*
 * Try to find a tuple received from the publication side (in 'remoteslot') in
 * the corresponding local relation using either replica identity index,
 * primary key or if needed, sequential scan.
 *
 * Local tuple, if found, is returned in '*localslot'.
 */
static bool
FindReplTupleInLocalRel(EState *estate, Relation localrel,

4.
@@ -2030,16 +2017,19 @@ apply_handle_delete_internal(ApplyExecutionData *edata,
 {
     EState       *estate = edata->estate;
     Relation    localrel = relinfo->ri_RelationDesc;
-    LogicalRepRelation *remoterel = &edata->targetRel->remoterel;
+    LogicalRepRelMapEntry *targetRel = edata->targetRel;
+    LogicalRepRelation *remoterel = &targetRel->remoterel;
     EPQState    epqstate;
     TupleTableSlot *localslot;

Do we need this change? I didn't see any place to use the variable targetRel
afterwards.

5.
+        if (!AttributeNumberIsValid(mainattno))
+        {
+            /*
+             * There are two cases to consider. First, if the index is a primary or
+             * unique key, we cannot have any indexes with expressions. So, at this
+             * point we are sure that the index we deal is not these.
+             */
+            Assert(RelationGetReplicaIndex(rel) != RelationGetRelid(idxrel) &&
+                   RelationGetPrimaryKeyIndex(rel) != RelationGetRelid(idxrel));
+
+            /*
+             * For a non-primary/unique index with an expression, we are sure that
+             * the expression cannot be used for replication index search. The
+             * reason is that we create relevant index paths by providing column
+             * equalities. And, the planner does not pick expression indexes via
+             * column equality restrictions in the query.
+             */
+            continue;
+        }

Is it possible that it is a usable index with an expression? I think indexes
with an expression has been filtered in 
FilterOutNotSuitablePathsForReplIdentFull(). If it can't be a usable index with
an expression, maybe we shouldn't use "continue" here.

6.
In the following case, I got a result which is different from HEAD, could you
please look into it?

-- publisher
CREATE TABLE test_replica_id_full (x int); 
ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL; 
CREATE PUBLICATION tap_pub_rep_full FOR TABLE test_replica_id_full;

-- subscriber
CREATE TABLE test_replica_id_full (x int, y int); 
CREATE INDEX test_replica_id_full_idx ON test_replica_id_full(x,y); 
CREATE SUBSCRIPTION tap_sub_rep_full_0 CONNECTION 'dbname=postgres port=5432' PUBLICATION tap_pub_rep_full;

-- publisher
INSERT INTO test_replica_id_full VALUES (1);
UPDATE test_replica_id_full SET x = x + 1 WHERE x = 1;

The data in subscriber:
on HEAD:
postgres=# select * from test_replica_id_full ;
 x | y
---+---
 2 |
(1 row)

After applying the patch:
postgres=# select * from test_replica_id_full ;
 x | y
---+---
 1 |
(1 row)

Regards,
Shi yu

Here are some review comments for the patch v8-0001:

======

1. Commit message

1a.
Majority of the logic on the subscriber side has already existed in the code.

SUGGESTION
The majority of the logic on the subscriber side already exists in the code.

~

1b.
Second, when REPLICA IDENTITY IS FULL on the publisher and an index is
used on the subscriber...

SUGGESTION
Second, when REPLICA IDENTITY FULL is on the publisher and an index is
used on the subscriber...

~

1c.
Still, below I try to show case the potential improvements using an
index on the subscriber
`pgbench_accounts(bid)`. With the index, the replication catches up
around ~5 seconds.
When the index is dropped, the replication takes around ~300 seconds.

"show case" -> "showcase"

~

1d.
In above text, what was meant by "catches up around ~5 seconds"?
e.g. Did it mean *improves* by ~5 seconds, or *takes* ~5 seconds?

~

1e.
// create one indxe, even on a low cardinality column

typo "indxe"

======

2. GENERAL

2a.
There are lots of single-line comments that start lowercase, but by
convention, I think they should start uppercase.

e.g. + /* we should always use at least one attribute for the index scan */
e.g. + /* we might not need this if the index is unique */
e.g. + /* avoid expensive equality check if index is unique */
e.g. + /* unrelated Path, skip */
e.g. + /* simple case, we already have an identity or pkey */
e.g. + /* indexscans are disabled, use seq. scan */
e.g. + /* target is a regular table */

~~

2b.
There are some excess blank lines between the function. By convention,
I think 1 blank line is normal, but here there are sometimes 2.

~~

2c.
There are some new function comments which include their function name
in the comment. It seemed unnecessary.

e.g. GetCheapestReplicaIdentityFullPath
e.g. FindUsableIndexForReplicaIdentityFull
e.g. LogicalRepUsableIndex

======

3. src/backend/executor/execReplication.c - build_replindex_scan_key

- int attoff;
+ int index_attoff;
+ int scankey_attoff;
  bool isnull;
  Datum indclassDatum;
  oidvector  *opclass;
  int2vector *indkey = &idxrel->rd_index->indkey;
- bool hasnulls = false;
-
- Assert(RelationGetReplicaIndex(rel) == RelationGetRelid(idxrel) ||
-    RelationGetPrimaryKeyIndex(rel) == RelationGetRelid(idxrel));

  indclassDatum = SysCacheGetAttr(INDEXRELID, idxrel->rd_indextuple,
  Anum_pg_index_indclass, &isnull);
  Assert(!isnull);
  opclass = (oidvector *) DatumGetPointer(indclassDatum);
+ scankey_attoff = 0;

Maybe just assign scankey_attoff = 0 at the declaration?

~~~

4.

+ /*
+ * There are two cases to consider. First, if the index is a primary or
+ * unique key, we cannot have any indexes with expressions. So, at this
+ * point we are sure that the index we deal is not these.
+ */

"we deal" -> "we are dealing with" ?

~~~

5.

+ /*
+ * For a non-primary/unique index with an additional expression, do
+ * not have to continue at this point. However, the below code
+ * assumes the index scan is only done for simple column references.
+ */
+ continue;

Is this one of those comments that ought to have a "XXX" prefix as a
note for the future?

~~~

6.

- int pkattno = attoff + 1;
...
  /* Initialize the scankey. */
- ScanKeyInit(&skey[attoff],
- pkattno,
+ ScanKeyInit(&skey[scankey_attoff],
+ index_attoff + 1,
  BTEqualStrategyNumber,
Wondering if it would have been simpler if you just did:
int pkattno = index_attoff + 1;

~~~

7.

- skey[attoff].sk_flags |= SK_ISNULL;
+ skey[scankey_attoff].sk_flags |= SK_ISNULL;
+ skey[scankey_attoff].sk_flags |= SK_SEARCHNULL;

SUGGESTION
skey[scankey_attoff].sk_flags |= (SK_ISNULL | SK_SEARCHNULL)

~~~

8. src/backend/executor/execReplication.c - RelationFindReplTupleByIndex

@@ -128,28 +171,44 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
  TransactionId xwait;
  Relation idxrel;
  bool found;
+ TypeCacheEntry **eq;
+ bool indisunique;
+ int scankey_attoff;

  /* Open the index. */
  idxrel = index_open(idxoid, RowExclusiveLock);
+ indisunique = idxrel->rd_index->indisunique;
+
+ /* we might not need this if the index is unique */
+ eq = NULL;

Maybe just default assign eq = NULL in the declaration?

~~~

9.

+ scan = index_beginscan(rel, idxrel, &snap,
+    scankey_attoff, 0);

Unnecessary wrapping?

~~~

10.

+ /* we only need to allocate once */
+ if (eq == NULL)
+ eq = palloc0(sizeof(*eq) * outslot->tts_tupleDescriptor->natts);

But shouldn't you also free this 'eq' before the function returns, to
prevent leaking memory?

======

11. src/backend/replication/logical/relation.c - logicalrep_rel_open

+ /*
+ * Finding a usable index is an infrequent operation, it is performed
+ * only when first time an operation is performed on the relation or
+ * after invalidation of the relation cache entry (e.g., such as ANALYZE).
+ */

SUGGESTION (minor rewording)
Finding a usable index is an infrequent task. It is performed only
when an operation is first performed on the relation, or after
invalidation of the relation cache entry (e.g., such as ANALYZE).

~~~

12. src/backend/replication/logical/relation.c - logicalrep_partition_open

Same as comment #11 above.

~~~

13. src/backend/replication/logical/relation.c - GetIndexOidFromPath

+static
+Oid
+GetIndexOidFromPath(Path *path)

Typically I think 'static Oid' should be on one line.

~~~

14.

+ switch (path->pathtype)
+ {
+ case T_IndexScan:
+ case T_IndexOnlyScan:
+ {
+ IndexPath  *index_sc = (IndexPath *) path;
+ indexOid = index_sc->indexinfo->indexoid;
+
+ break;
+ }
+
+ default:
+ indexOid = InvalidOid;
+ }

Is there any point in using a switch statement when there is only one
functional code block?

Why not just do:

if (path->pathtype == T_IndexScan || path->pathtype == T_IndexOnlyScan)
{
...
}

return InvalidOid;

~~~

15. src/backend/replication/logical/relation.c - IndexOnlyOnExpression

+ * Returns true if the given index consist only of expressions such as:
+ * CREATE INDEX idx ON table(foo(col));

"consist" -> "consists"

~~~

16.

+IndexOnlyOnExpression(IndexInfo *indexInfo)
+{
+ int i=0;
+ for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)

Don't initialise 'i' twice.

~~~

17.

+ AttrNumber attnum = indexInfo->ii_IndexAttrNumbers[i];
+ if (AttributeNumberIsValid(attnum))
+ return false;
+
+ }

Spurious blank line

~~~

18. src/backend/replication/logical/relation.c -
GetCheapestReplicaIdentityFullPath

+/*
+ * Iterates over the input path list, and returns another path list
+ * where paths with non-btree indexes, partial indexes or
+ * indexes on only expressions are eliminated from the list.
+ */

"path list, and" -> "path list and"

~~~

19.

+ if (!OidIsValid(indexOid))
+ {
+ /* unrelated Path, skip */
+ suitableIndexList = lappend(suitableIndexList, path);
+ continue;
+ }
+
+ indexRelation = index_open(indexOid, AccessShareLock);
+ indexInfo = BuildIndexInfo(indexRelation);
+ is_btree_index = (indexInfo->ii_Am == BTREE_AM_OID);
+ is_partial_index = (indexInfo->ii_Predicate != NIL);
+ is_index_only_on_expression = IndexOnlyOnExpression(indexInfo);
+ index_close(indexRelation, NoLock);
+
+ if (!is_btree_index || is_partial_index || is_index_only_on_expression)
+ continue;

Maybe better to change this logic using if/else and changing the last
condition so them you can avoid having any of those 'continue' in this
loop.

~~~

20. src/backend/replication/logical/relation.c -
GetCheapestReplicaIdentityFullPath

+/*
+ * GetCheapestReplicaIdentityFullPath generates all the possible paths
+ * for the given subscriber relation, assuming that the source relation
+ * is replicated via REPLICA IDENTITY FULL.
+ *
+ * The function assumes that all the columns will be provided during
+ * the execution phase, given that REPLICA IDENTITY FULL gurantees
+ * that.
+ */

20a.
typo "gurantees"

~

20b.
The function comment neglects to say that after getting all these
paths the final function return is the cheapest one that it found.

~~~

21.

+ for (attno = 0; attno < RelationGetNumberOfAttributes(localrel); attno++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(localrel->rd_att, attno);
+
+ if (attr->attisdropped)
+ {
+ continue;
+ }
+ else
+ {
+ Expr    *eq_op;

Maybe simplify by just removing the 'else' or instead just reverse the
condition of the 'if'.

~~~

22.

+ /*
+ * A sequential scan has could have been dominated by
+ * by an index scan during make_one_rel(). We should always
+ * have a sequential scan before set_cheapest().
+ */

"has could have been" -> "could have been"

~~~

23. src/backend/replication/logical/relation.c - LogicalRepUsableIndex

+static Oid
+LogicalRepUsableIndex(Relation localrel, LogicalRepRelation *remoterel)
+{
+ Oid idxoid;
+
+ /*
+ * We never need index oid for partitioned tables, always rely on leaf
+ * partition's index.
+ */
+ if (localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ return InvalidOid;
+
+ /* simple case, we already have an identity or pkey */
+ idxoid = GetRelationIdentityOrPK(localrel);
+ if (OidIsValid(idxoid))
+ return idxoid;
+
+ /* indexscans are disabled, use seq. scan */
+ if (!enable_indexscan)
+ return InvalidOid;

I thought the (!enable_indexscan) fast exit perhaps should be done
first, or at least before calling GetRelationIdentityOrPK.

======

24. src/backend/replication/logical/worker.c - apply_handle_delete_internal

@@ -2034,12 +2021,14 @@ apply_handle_delete_internal(ApplyExecutionData *edata,
  EPQState epqstate;
  TupleTableSlot *localslot;
  bool found;
+ Oid usableIndexOid = usable_indexoid_internal(edata, relinfo);

  EvalPlanQualInit(&epqstate, estate, NULL, NIL, -1);
  ExecOpenIndices(relinfo, false);

- found = FindReplTupleInLocalRel(estate, localrel, remoterel,
- remoteslot, &localslot);
+
+ found = FindReplTupleInLocalRel(estate, localrel, usableIndexOid,
+ remoterel, remoteslot, &localslot);

24a.
Excess blank line above FindReplTupleInLocalRel call.

~

24b.
This code is almost same in function handle_update_internal(), except
the wrapping of the params is different. Better to keep everything
consistent looking.

~~~

25. src/backend/replication/logical/worker.c - usable_indexoid_internal

+/*
+ * Decide whether we can pick an index for the relinfo (e.g., the relation)
+ * we're actually deleting/updating from. If it is a child partition of
+ * edata->targetRelInfo, find the index on the partition.
+ */
+static Oid
+usable_indexoid_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo)

I'm not sure is this can maybe return InvalidOid? The function comment
should clarify it.

~~~

26.

I might be mistaken, but somehow I feel this function can be
simplified. e.g. If you have a var 'relmapentry' and let the normal
table use the initial value of that. Then I think you only need to
test for the partitioned table and reassign that var as appropriate.
It also removes the need for having 'usableIndexOid' var.

FOR EXAMPLE,

static Oid
usable_indexoid_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo)
{
ResultRelInfo *targetResultRelInfo = edata->targetRelInfo;
LogicalRepRelMapEntry *relmapentry = edata->targetRel;
Oid targetrelid = targetResultRelInfo->ri_RelationDesc->rd_rel->oid;
Oid localrelid = relinfo->ri_RelationDesc->rd_id;

if (targetrelid != localrelid)
{
/*
* Target is a partitioned table, so find relmapentry of the partition.
*/
TupleConversionMap *map = relinfo->ri_RootToPartitionMap;
AttrMap    *attrmap = map ? map->attrMap : NULL;
LogicalRepPartMapEntry *part_entry =
logicalrep_partition_open(relmapentry, relinfo->ri_RelationDesc,
attrmap);

Assert(targetResultRelInfo->ri_RelationDesc->rd_rel->relkind ==
   RELKIND_PARTITIONED_TABLE);

relmapentry = part_entry->relmapentry;
}
return relmapentry->usableIndexOid;
}

~~~

27.

+ /*
+ * Target is a partitioned table, get the index oid the partition.
+ */

SUGGESTION
Target is a partitioned table, so get the index oid of the partition.

or (see the example of comment @26)

~~~

28. src/backend/replication/logical/worker.c - FindReplTupleInLocalRel

@@ -2093,12 +2125,11 @@ FindReplTupleInLocalRel(EState *estate,
Relation localrel,

  *localslot = table_slot_create(localrel, &estate->es_tupleTable);

I think this might have been existing functionality...

The comment says "* Local tuple, if found, is returned in
'*localslot'." But the code is unconditionally doing
table_slot_create() before it even knows if a tuple was found or not.
So what about when it is NOT found - in that case shouldn't there be
some cleaning up that (unused?) table slot that got unconditionally
created?

~~~

29. src/backend/replication/logical/worker.c - apply_handle_tuple_routing

@@ -2202,13 +2233,17 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
  * suitable partition.
  */
  {
+ LogicalRepRelMapEntry *entry;
  TupleTableSlot *localslot;
  ResultRelInfo *partrelinfo_new;
  bool found;

+ entry = &part_entry->relmapentry;

Maybe just do this assignment at the entry declaration time?

~~~

30.

  /* Get the matching local tuple from the partition. */
  found = FindReplTupleInLocalRel(estate, partrel,
- &part_entry->remoterel,
+ part_entry->relmapentry.usableIndexOid,
+ &entry->remoterel,
  remoteslot_part, &localslot);
Why not use the new 'entry' var just assigned instead of repeating
part_entry->relmapentry?

SUGGESTION
found = FindReplTupleInLocalRel(estate, partrel,
entry->usableIndexOid,
&entry->remoterel,
remoteslot_part, &localslot);

~~~

31.

+ slot_modify_data(remoteslot_part, localslot, entry,
  newtup);

Unnecessary wrapping.

======

32. src/include/replication/logicalrelation.h

+typedef struct LogicalRepPartMapEntry
+{
+ Oid partoid; /* LogicalRepPartMap's key */
+ LogicalRepRelMapEntry relmapentry;
+} LogicalRepPartMapEntry;

IIUC this struct has been moved from relation.c to here. But I think
there was a large comment about this struct which maybe needs to be
moved with it (see the original relation.c).

/*
 * Partition map (LogicalRepPartMap)
 *
 * When a partitioned table is used as replication target, replicated
 * operations are actually performed on its leaf partitions, which requires
 * the partitions to also be mapped to the remote relation.  Parent's entry
 * (LogicalRepRelMapEntry) cannot be used as-is for all partitions, because
 * individual partitions may have different attribute numbers, which means
 * attribute mappings to remote relation's attributes must be maintained
 * separately for each partition.
 */

======

33. .../subscription/t/032_subscribe_use_index.pl

Typo "MULTIPILE"

This typo occurs several times...

e.g. # Testcase start: SUBSCRIPTION USES INDEX MODIFIES MULTIPILE ROWS
e.g. # Testcase end: SUBSCRIPTION USES INDEX MODIFIES MULTIPILE ROWS
e.g. # Testcase start: SUBSCRIPTION USES INDEX WITH MULTIPILE COLUMNS
e.g. # Testcase end: SUBSCRIPTION USES INDEX MODIFIES MULTIPILE ROWS

~~~

34.

# Basic test where the subscriber uses index
# and only touches multiple rows

What does "only ... multiple" mean?

This occurs several times also.

~~~

35.

+# wait for initial table synchronization to finish
+$node_subscriber->wait_for_subscription_sync;
+$node_subscriber->wait_for_subscription_sync;
+$node_subscriber->wait_for_subscription_sync;

That triple wait looks unusual. Is it deliberate?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Hi Onder,

Since you ask me several questions [1], this post is just for answering those.

I have looked again at the latest v9 patch, but I will post my review
comments for that separately.


On Thu, Aug 25, 2022 at 7:09 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
>> 1d.
>> In above text, what was meant by "catches up around ~5 seconds"?
>> e.g. Did it mean *improves* by ~5 seconds, or *takes* ~5 seconds?
>>
>
> It "takes" 5 seconds to replicate all the changes. To be specific, I execute 'SELECT sum(abalance) FROM
pgbench_accounts'on the subscriber, and try to measure the time until when all the changes are replicated. I do use the
samequery on the publisher to check what the query result should be when replication is done. 
>
> I updated the relevant text, does that look better?

Yes.

>> 2. GENERAL
>>
>> 2a.
>> There are lots of single-line comments that start lowercase, but by
>> convention, I think they should start uppercase.
>>
>> e.g. + /* we should always use at least one attribute for the index scan */
>> e.g. + /* we might not need this if the index is unique */
>> e.g. + /* avoid expensive equality check if index is unique */
>> e.g. + /* unrelated Path, skip */
>> e.g. + /* simple case, we already have an identity or pkey */
>> e.g. + /* indexscans are disabled, use seq. scan */
>> e.g. + /* target is a regular table */
>>
>> ~~
>
>
> Thanks for noting this, I didn't realize that there is a strict requirement on this. Updated all of your suggestions,
andrealized one more such case. 
>
> Is there documentation where such conventions are listed? I couldn't find any.

I don’t know of any strict requirements, but I did think it was the
more common practice to make the comments look like proper sentences.
However, when I tried to prove that by counting the single-line
comments in PG code it seems to be split almost 50:50
lowercase/uppercase, so I guess you should just do whatever is most
sensible or is most consistent with the surrounding code ….

Counts for single line /* */ comments:
regex ^\s*\/\*\s[a-z]+.*\*\/$  = 18222 results
regex ^\s*\/\*\s[A-Z]+.*\*\/$ = 20252 results

>> 3. src/backend/executor/execReplication.c - build_replindex_scan_key
>>
>> - int attoff;
>> + int index_attoff;
>> + int scankey_attoff;
>>   bool isnull;
>>   Datum indclassDatum;
>>   oidvector  *opclass;
>>   int2vector *indkey = &idxrel->rd_index->indkey;
>> - bool hasnulls = false;
>> -
>> - Assert(RelationGetReplicaIndex(rel) == RelationGetRelid(idxrel) ||
>> -    RelationGetPrimaryKeyIndex(rel) == RelationGetRelid(idxrel));
>>
>>   indclassDatum = SysCacheGetAttr(INDEXRELID, idxrel->rd_indextuple,
>>   Anum_pg_index_indclass, &isnull);
>>   Assert(!isnull);
>>   opclass = (oidvector *) DatumGetPointer(indclassDatum);
>> + scankey_attoff = 0;
>>
>> Maybe just assign scankey_attoff = 0 at the declaration?
>>
>
> Again, lack of coding convention knowledge :/ My observation is that it is often not assigned during the declaration.
But,changed this one. 
>

I don’t know of any convention. Probably this is just my own
preference to keep the simple default assignments with the declaration
to reduce the LOC. YMMV.

>>
>> 6.
>>
>> - int pkattno = attoff + 1;
>> ...
>>   /* Initialize the scankey. */
>> - ScanKeyInit(&skey[attoff],
>> - pkattno,
>> + ScanKeyInit(&skey[scankey_attoff],
>> + index_attoff + 1,
>>   BTEqualStrategyNumber,
>> Wondering if it would have been simpler if you just did:
>> int pkattno = index_attoff + 1;
>
>
>
> The index is not necessarily the primary key at this point, that's why I removed it.
>
> There are already 3 variables in the same function index_attoff, scankey_attoff and table_attno, which are hard to
avoid.But, this one seemed ok to avoid, mostly to simplify the readability. Do you think it is better with the
additionalvariable? Still, I think we need a better name as "pk" is not relevant anymore. 
>

Your code is fine. Leave it as-is.

>> 8. src/backend/executor/execReplication.c - RelationFindReplTupleByIndex
>>
>> @@ -128,28 +171,44 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
>>   TransactionId xwait;
>>   Relation idxrel;
>>   bool found;
>> + TypeCacheEntry **eq;
>> + bool indisunique;
>> + int scankey_attoff;
>>
>>   /* Open the index. */
>>   idxrel = index_open(idxoid, RowExclusiveLock);
>> + indisunique = idxrel->rd_index->indisunique;
>> +
>> + /* we might not need this if the index is unique */
>> + eq = NULL;
>>
>> Maybe just default assign eq = NULL in the declaration?
>>
>
> Again, I wasn't sure if it is OK regarding the coding convention to assign during the declaration. Changed now.
>

Same as #3.

>> 10.
>>
>> + /* we only need to allocate once */
>> + if (eq == NULL)
>> + eq = palloc0(sizeof(*eq) * outslot->tts_tupleDescriptor->natts);
>>
>> But shouldn't you also free this 'eq' before the function returns, to
>> prevent leaking memory?
>>
>
> Two notes here. First, this is allocated inside ApplyMessageContext, which seems to be reset per tuple change. So,
thatseems like a good boundary to keep this allocation in memory. 
>

OK, fair enough. Is it worth adding a comment to say that or not?

> Second, RelationFindReplTupleSeq() doesn't free the same allocation roughly at a very similar call stack. That's why
Idecided not to pfree. Do you see strong reason to pfree at this point? Then we should probably change that for
RelationFindReplTupleSeq()as well. 
>
>>
>> 23. src/backend/replication/logical/relation.c - LogicalRepUsableIndex
>>
>> +static Oid
>> +LogicalRepUsableIndex(Relation localrel, LogicalRepRelation *remoterel)
>> +{
>> + Oid idxoid;
>> +
>> + /*
>> + * We never need index oid for partitioned tables, always rely on leaf
>> + * partition's index.
>> + */
>> + if (localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>> + return InvalidOid;
>> +
>> + /* simple case, we already have an identity or pkey */
>> + idxoid = GetRelationIdentityOrPK(localrel);
>> + if (OidIsValid(idxoid))
>> + return idxoid;
>> +
>> + /* indexscans are disabled, use seq. scan */
>> + if (!enable_indexscan)
>> + return InvalidOid;
>>
>> I thought the (!enable_indexscan) fast exit perhaps should be done
>> first, or at least before calling GetRelationIdentityOrPK.
>
>
> This is actually a point where I need some more feedback. On HEAD, even if the index scan is disabled, we use the
index.For this one, (a) I didn't want to change the behavior for existing users (b) want to have a way to disable this
feature,and enable_indexscan seems like a good one. 
>
> Do you think I should dare to move it above GetRelationIdentityOrPK()? Or, maybe I just need more comments? I
improvedthe comment, and it would be nice to hear your thoughts on this. 

I agree with you it is maybe best not to cause any changes in
behaviour. If the behaviour is unwanted then it should be changed
independently of this patch anyhow.

>> 24b.
>> This code is almost same in function handle_update_internal(), except
>> the wrapping of the params is different. Better to keep everything
>> consistent looking.
>>
>
> Hmm, I have not changed how they look because they have one variable difference (&relmapentry->remoterel vs
remoterel),which requires the indentation to be slightly difference. So, I either need a new variable or keep them
as-is?

OK. Keep code as-is.

>>
>> 28. src/backend/replication/logical/worker.c - FindReplTupleInLocalRel
>>
>> @@ -2093,12 +2125,11 @@ FindReplTupleInLocalRel(EState *estate,
>> Relation localrel,
>>
>>   *localslot = table_slot_create(localrel, &estate->es_tupleTable);
>>
>> I think this might have been existing functionality...
>>
>> The comment says "* Local tuple, if found, is returned in
>> '*localslot'." But the code is unconditionally doing
>> table_slot_create() before it even knows if a tuple was found or not.
>> So what about when it is NOT found - in that case shouldn't there be
>> some cleaning up that (unused?) table slot that got unconditionally
>> created?
>>
>
> This sounds accurate. But I guess it may not have been considered critical as we are operating in the
ApplyMessageContext?Tha is going to be freed once a single tuple is dispatched. 
>
> I have a slight preference not to do it in this patch, but if you think otherwise let me know.

I agree. Maybe this is not even a leak worth bothering about if it is
only in the short-lived ApplyMessageContext like you say. Anyway,
AFAIK this was already in existing code, so a fix (if any) would
belong in a different patch to this one.

>> 31.
>>
>> + slot_modify_data(remoteslot_part, localslot, entry,
>>   newtup);
>>
>> Unnecessary wrapping.
>>
>> ======
>
>
> I think I have not changed this, but fixed anyway

Hmm - I don't see that you changed this, but anyway I guess you
shouldn't be fixing wrapping problems unless this patch caused them.

------
[1] https://www.postgresql.org/message-id/CACawEhXbw%3D%3DK02v3%3DnHFEAFJqegx0b4r2J%2BFtXtKFkJeE6R95Q%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Here are some review comments for the patch v9-0001:

======

1. Commit message

1a.
With this patch, I'm proposing the following change: If there is an
index on the subscriber, use the index as long as the planner
sub-modules picks any index over sequential scan. The index should be
a btree index, not a partital index. Finally, the index should have at
least one column reference (e.g., cannot consists of only
expressions).

SUGGESTION
With this patch, I'm proposing the following change: If there is any
index on the subscriber, let the planner sub-modules compare the costs
of index versus sequential scan and choose the cheapest. The index
should be a btree index, not a partial index, and it should have at
least one column reference (e.g., cannot consist of only expressions).

~

1b.
The Majority of the logic on the subscriber side exists in the code.

"exists" -> "already exists"

~

1c.
psql -c "truncate pgbench_accounts;" -p 9700 postgres

"truncate" -> "TRUNCATE"

~

1d.
Try to wrap this message text at 80 char width.

======

2. src/backend/replication/logical/relation.c - logicalrep_rel_open

+ /*
+ * Finding a usable index is an infrequent task. It is performed
+ * when an operation is first performed on the relation, or after
+ * invalidation of the relation cache entry (e.g., such as ANALYZE).
+ */
+ entry->usableIndexOid = LogicalRepUsableIndex(entry->localrel, remoterel);

Seemed a bit odd to say "performed" 2x in the same sentence.

"It is performed when..." -> "It occurs when...” (?)

~~~

3. src/backend/replication/logical/relation.c - logicalrep_partition_open

+ /*
+ * Finding a usable index is an infrequent task. It is performed
+ * when an operation is first performed on the relation, or after
+ * invalidation of the relation cache entry (e.g., such as ANALYZE).
+ */
+ part_entry->relmapentry.usableIndexOid =
+ LogicalRepUsableIndex(partrel, remoterel);

3a.
Same as comment #2 above.

~

3b.
The jumping between 'part_entry' and 'entry' is confusing. Since
'entry' is already assigned to be &part_entry->relmapentry can't you
use that here?

SUGGESTION
entry->usableIndexOid = LogicalRepUsableIndex(partrel, remoterel);

~~~

4. src/backend/replication/logical/relation.c - GetIndexOidFromPath

+/*
+ * Returns a valid index oid if the input path is an index path.
+ * Otherwise, return invalid oid.
+ */
+static Oid
+GetIndexOidFromPath(Path *path)

Perhaps may this function comment more consistent with others (like
GetRelationIdentityOrPK, LogicalRepUsableIndex) and refer to the
InvalidOid.

SUGGESTION
/*
 * Returns a valid index oid if the input path is an index path.
 *
 * Otherwise, returns InvalidOid.
 */

~~~

5. src/backend/replication/logical/relation.c - IndexOnlyOnExpression

+bool
+IndexOnlyOnExpression(IndexInfo *indexInfo)
+{
+ int i;
+ for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
+ {
+ AttrNumber attnum = indexInfo->ii_IndexAttrNumbers[i];
+ if (AttributeNumberIsValid(attnum))
+ return false;
+ }
+
+ return true;
+}

5a.
Add a blank line after those declarations.

~

5b.
AFAIK the C99 style for loop declarations should be OK [1] for new
code, so declaring like below would be cleaner:

for (int i = 0; ...

~~~

6. src/backend/replication/logical/relation.c -
FilterOutNotSuitablePathsForReplIdentFull

+/*
+ * Iterates over the input path list and returns another path list
+ * where paths with non-btree indexes, partial indexes or
+ * indexes on only expressions are eliminated from the list.
+ */
+static List *
+FilterOutNotSuitablePathsForReplIdentFull(List *pathlist)

"are eliminated from the list." -> "have been removed."

~~~

7.

+ foreach(lc, pathlist)
+ {
+ Path    *path = (Path *) lfirst(lc);
+ Oid indexOid = GetIndexOidFromPath(path);
+ Relation indexRelation;
+ IndexInfo *indexInfo;
+ bool is_btree;
+ bool is_partial;
+ bool is_only_on_expression;
+
+ if (!OidIsValid(indexOid))
+ {
+ /* Unrelated Path, skip */
+ suitableIndexList = lappend(suitableIndexList, path);
+ }
+ else
+ {
+ indexRelation = index_open(indexOid, AccessShareLock);
+ indexInfo = BuildIndexInfo(indexRelation);
+ is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
+ is_partial = (indexInfo->ii_Predicate != NIL);
+ is_only_on_expression = IndexOnlyOnExpression(indexInfo);
+ index_close(indexRelation, NoLock);
+
+ if (is_btree && !is_partial && !is_only_on_expression)
+ suitableIndexList = lappend(suitableIndexList, path);
+ }
+ }

I think most of those variables are only used in the "else" block so
maybe it's better to declare them at that scope.

+ Relation indexRelation;
+ IndexInfo *indexInfo;
+ bool is_btree;
+ bool is_partial;
+ bool is_only_on_expression;

~~~

8. src/backend/replication/logical/relation.c -
GetCheapestReplicaIdentityFullPath

+ * Indexes that consists of only expressions (e.g.,
+ * no simple column references on the index) are also
+ * eliminated with a similar reasoning.

"consists" -> "consist"

"with a similar reasoning" -> "with similar reasoning"

~~~

9.

+ * We also eliminate non-btree indexes, which could be relaxed
+ * if needed. If we allow non-btree indexes, we should adjust
+ * RelationFindReplTupleByIndex() to support such indexes.

This looks like another of those kinds of comments that should have
"XXX" prefix as a note to the future.

~~~

10. src/backend/replication/logical/relation.c -
FindUsableIndexForReplicaIdentityFull

+/*
+ * Returns an index oid if the planner submodules picks index scans
+ * over sequential scan.

10a
"picks" -> "pick"

~

10b.
Maybe this should also say ", otherwise returns InvalidOid" (?)

~~~

11.

+FindUsableIndexForReplicaIdentityFull(Relation localrel)
+{
+ MemoryContext usableIndexContext;
+ MemoryContext oldctx;
+ Path *cheapest_total_path;
+ Oid indexOid;

In the following function, and in the one after that, you've named the
index Oid as 'idxoid' (not 'indexOid'). IMO it's better to use
consistent naming everywhere.

~~~

12. src/backend/replication/logical/relation.c - GetRelationIdentityOrPK

12a.
I wondered what is the benefit of having this function. IIUC it is
only called from one place (LogicalRepUsableIndex) and IMO the code
would probably be easier if you just inline this logic in that
function...

~

12b.
+/*
+ * Get replica identity index or if it is not defined a primary key.
+ *
+ * If neither is defined, returns InvalidOid
+ */

If you want to keep the function for some reason (e.g. see #12a) then
I thought the function comment could be better.

SUGGESTION
/*
 * Returns OID of the relation's replica identity index, or OID of the
 * relation's primary key index.
 *
 * If neither is defined, returns InvalidOid.
 */

~~~

13. src/backend/replication/logical/relation.c - LogicalRepUsableIndex

For some reason, I feel this function should be called
FindLogicalRepUsableIndex (or similar), because it seems more
consistent with the others which might return the Oid or might return
InvalidOid...

~~~

14.

+ /*
+ * Index scans are disabled, use sequential scan. Note that we do allow
+ * index scans when there is a primary key or unique index replica
+ * identity. That is the legacy behavior so we hesitate to move this check
+ * above.
+ */

Perhaps a slight rephrasing of that comment?

SUGGESTION
If index scans are disabled, use a sequential scan.

Note that we still allowed index scans above when there is a primary
key or unique index replica identity, but that is the legacy behaviour
(even when enable_indexscan is false), so we hesitate to move this
enable_indexscan check to be done earlier in this function.

~~~

15.

+ * If we had a primary key or relation identity with a unique index,
+ * we would have already found a valid oid. At this point, the remote
+ * relation has replica identity full and we have at least one local
+ * index defined.

"would have already found a valid oid." -> "would have already found
and returned that oid."

======

16. src/backend/replication/logical/worker.c - usable_indexoid_internal

+/*
+ * Decide whether we can pick an index for the relinfo (e.g., the relation)
+ * we're actually deleting/updating from. If it is a child partition of
+ * edata->targetRelInfo, find the index on the partition.
+ *
+ * Note that if the corresponding relmapentry has InvalidOid usableIndexOid,
+ * the function returns InvalidOid. In that case, the tuple is used via
+ * sequential execution.
+ */
+static Oid
+usable_indexoid_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo)

I am not sure this is the right place to be saying that last sentence
("In that case, the tuple is used via sequential execution.") because
it's up to the *calling* code to decide what to do if InvalidOid is
returned

======

17. src/include/replication/logicalrelation.h

@ -31,20 +32,40 @@ typedef struct LogicalRepRelMapEntry
  Relation localrel; /* relcache entry (NULL when closed) */
  AttrMap    *attrmap; /* map of local attributes to remote ones */
  bool updatable; /* Can apply updates/deletes? */
+ Oid usableIndexOid; /* which index to use? (Invalid when no index
+ * used) */

SUGGESTION (for the comment)
which index to use, or InvalidOid if none

~~~

18.

+/*
+ * Partition map (LogicalRepPartMap)
+ *
+ * When a partitioned table is used as replication target, replicated
+ * operations are actually performed on its leaf partitions, which requires
+ * the partitions to also be mapped to the remote relation.  Parent's entry
+ * (LogicalRepRelMapEntry) cannot be used as-is for all partitions, because
+ * individual partitions may have different attribute numbers, which means
+ * attribute mappings to remote relation's attributes must be maintained
+ * separately for each partition.
+ */
+typedef struct LogicalRepPartMapEntry

Something feels not quite right using the (unchanged) comment about
the Partition map which was removed from where it was originally in
relation.c.

The reason I am unsure is that this comment is still referring to the
"LogicalRepPartMap", which is not here but is declared static in
relation.c. Maybe the quick/easy fix would be to just change the first
line to say: "Partition map (see LogicalRepPartMap in relation.c)".
OTOH, I'm not sure if some part of this comment still needs to be left
in relation.c (??)


------
[1] https://www.postgresql.org/docs/devel/source-conventions.html

Kind Regards,
Peter Smith.
Fujitsu Australia

On Sat, Aug 20, 2022 at 4:32 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> I'm a little late to catch up with your comments, but here are my replies:
>
>> > My answer for the above assumes that your question is regarding what happens if you ANALYZE on a partitioned
table.If your question is something different, please let me know. 
>> >
>>
>> I was talking about inheritance cases, something like:
>> create table tbl1 (a int);
>> create table tbl1_part1 (b int) inherits (tbl1);
>> create table tbl1_part2 (c int) inherits (tbl1);
>>
>> What we do in such cases is documented as: "if the table being
>> analyzed has inheritance children, ANALYZE gathers two sets of
>> statistics: one on the rows of the parent table only, and a second
>> including rows of both the parent table and all of its children. This
>> second set of statistics is needed when planning queries that process
>> the inheritance tree as a whole. The child tables themselves are not
>> individually analyzed in this case."
>
>
> Oh, I haven't considered inherited tables. That seems right, the statistics of the children are not updated when the
parentis analyzed. 
>
>>
>> Now, the point I was worried about was what if the changes in child
>> tables (*_part1, *_part2) are much more than in tbl1? In such cases,
>> we may not invalidate child rel entries, so how will logical
>> replication behave for updates/deletes on child tables? There may not
>> be any problem here but it is better to do some analysis of such cases
>> to see how it behaves.
>
>
> I also haven't observed any specific issues. In the end, when the user (or autovacuum) does ANALYZE on the child, it
iswhen the statistics are updated for the child. 
>

Right, I also think that should be the behavior but I have not
verified it. However, I think it should be easy to verify if
autovacuum updates the stats for child tables when we operate on only
one of such tables and whether that will invalidate the cache for our
case.

> Although I do not have much experience with inherited tables, this sounds like the expected behavior?
>
> I also pushed a test covering inherited tables. First, a basic test on the parent. Then, show that updates on the
parentcan also use indexes of the children. Also, after an ANALYZE on the child, we can re-calculate the index and use
theindex with a higher cardinality column. 
>
>>
>> > Also, for the majority of the use-cases, I think we'd probably expect an index on a column with high cardinality
--hence use index scan. So, bitmap index scans are probably not going to be that much common. 
>> >
>>
>> You are probably right here but I don't think we can make such
>> assumptions. I think the safest way to avoid any regression here is to
>> choose an index when the planner selects an index scan. We can always
>> extend it later to bitmap scans if required. We can add a comment
>> indicating the same.
>>
>
> Alright, I got rid of the bitmap scans.
>
> Though, it caused few of the new tests to fail. I think because of the data size/distribution, the planner picks
bitmapscans. To make the tests consistent and small, I added `enable_bitmapscan to off` for this new test file. Does
thatsound ok to you? Or, should we change the tests to make sure they genuinely use index scans? 
>

That sounds okay to me.

--
With Regards,
Amit Kapila.

Dear  Önder,

Thank you for proposing good feature. I'm also interested in the patch, 
So I started to review this. Followings are initial comments.

===
For execRelation.c

01. RelationFindReplTupleByIndex()

```
        /* Start an index scan. */
        InitDirtySnapshot(snap);
-       scan = index_beginscan(rel, idxrel, &snap,
-                                                  IndexRelationGetNumberOfKeyAttributes(idxrel),
-                                                  0);
 
        /* Build scan key. */
-       build_replindex_scan_key(skey, rel, idxrel, searchslot);
+       scankey_attoff = build_replindex_scan_key(skey, rel, idxrel, searchslot);
 
+       scan = index_beginscan(rel, idxrel, &snap, scankey_attoff, 0);
```

I think "/* Start an index scan. */" should be just above index_beginscan().

===
For worker.c

02. sable_indexoid_internal()

```
+ * Note that if the corresponding relmapentry has InvalidOid usableIndexOid,
+ * the function returns InvalidOid.
+ */
+static Oid
+usable_indexoid_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo)
```

"InvalidOid usableIndexOid" should be "invalid usableIndexOid,"

03. check_relation_updatable()

```
         * We are in error mode so it's fine this is somewhat slow. It's better to
         * give user correct error.
         */
-       if (OidIsValid(GetRelationIdentityOrPK(rel->localrel)))
+       if (OidIsValid(rel->usableIndexOid))
        {
```

Shouldn't we change the above comment to? The check is no longer slow.

===
For relation.c

04. GetCheapestReplicaIdentityFullPath()

```
+static Path *
+GetCheapestReplicaIdentityFullPath(Relation localrel)
+{
+       PlannerInfo *root;
+       Query      *query;
+       PlannerGlobal *glob;
+       RangeTblEntry *rte;
+       RelOptInfo *rel;
+       int     attno;
+       RangeTblRef *rt;
+       List *joinList;
+       Path *seqScanPath;
```

I think the part that constructs dummy-planner state can be move to another function
because that part is not meaningful for this.
Especially line 824-846 can. 


===
For 032_subscribe_use_index.pl

05. general

```
+# insert some initial data within the range 0-1000
+$node_publisher->safe_psql('postgres',
+       "INSERT INTO test_replica_id_full SELECT i%20 FROM generate_series(0,1000)i;"
+);
```

It seems that the range of initial data seems [0, 19].
Same mistake-candidates are found many place.

06. general

```
+# updates 1000 rows
+$node_publisher->safe_psql('postgres',
+       "UPDATE test_replica_id_full SET x = x + 1 WHERE x = 15;");
```

Only 50 tuples are modified here.
Same mistake-candidates are found many place.

07. general

```
+# we check if the index is used or not
+$node_subscriber->poll_query_until(
+       'postgres', q{select (idx_scan = 200) from pg_stat_all_indexes where indexrelname =
'test_replica_id_full_idx';}
+) or die "Timed out while waiting for check subscriber tap_sub_rep_full_3 updates 200 rows via index";    
```
The query will be executed until the index scan is finished, but it may be not commented.
How about changing it to "we wait until the index used on the subscriber-side." or something?
Same comments are found in many place.

08. test related with ANALYZE

```
+# Testcase start: SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE - PARTITIONED TABLE
+# ====================================================================
```

"Testcase start:" should be "Testcase end:" here.

09. general

In some tests results are confirmed but in other test they are not.
I think you can make sure results are expected in any case if there are no particular reasons.


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Here are some review comments for the latest v10 patch.

(Mostly these are just nitpick wording/comments etc)

======

1. Commit message

It is often not feasible to use `REPLICA IDENTITY FULL` on the publication
because it leads to full table scan per tuple change on the subscription.
This makes `REPLICA IDENTITY FULL` impracticable -- probably other than
some small number of use cases.

~

The "often not feasible" part seems repeated by the "impracticable" part.

SUGGESTION
Using `REPLICA IDENTITY FULL` on the publication leads to a full table
scan per tuple change on the subscription. This makes `REPLICA
IDENTITY FULL` impracticable -- probably other than some small number
of use cases.

~~~

2.

The Majority of the logic on the subscriber side already exists in
the code.

"Majority" -> "majority"

~~~

3.

The ones familiar
with this part of the code could realize that the sequential scan
code on the subscriber already implements the `tuples_equal()`
function.

SUGGESTION
Anyone familiar with this part of the code might recognize that...

~~~

4.

In short, the changes on the subscriber is mostly
combining parts of (unique) index scan and sequential scan codes.

"is mostly" -> "are mostly"

~~~

5.

From the performance point of view, there are few things to note.

"are few" -> "are a few"

======

6. src/backend/executor/execReplication.c - build_replindex_scan_key

+static int
 build_replindex_scan_key(ScanKey skey, Relation rel, Relation idxrel,
  TupleTableSlot *searchslot)
 {
- int attoff;
+ int index_attoff;
+ int scankey_attoff = 0;

Should it be called 'skey_attoff' for consistency with the param 'skey'?

~~~

7.

  Oid operator;
  Oid opfamily;
  RegProcedure regop;
- int pkattno = attoff + 1;
- int mainattno = indkey->values[attoff];
- Oid optype = get_opclass_input_type(opclass->values[attoff]);
+ int table_attno = indkey->values[index_attoff];
+ Oid optype = get_opclass_input_type(opclass->values[index_attoff]);

Maybe the 'optype' should be adjacent to the other Oid opXXX
declarations just to keep them all together?

~~~

8.

+ if (!AttributeNumberIsValid(table_attno))
+ {
+ IndexInfo *indexInfo PG_USED_FOR_ASSERTS_ONLY;
+
+ /*
+ * There are two cases to consider. First, if the index is a primary or
+ * unique key, we cannot have any indexes with expressions. So, at this
+ * point we are sure that the index we are dealing with is not these.
+ */
+ Assert(RelationGetReplicaIndex(rel) != RelationGetRelid(idxrel) &&
+    RelationGetPrimaryKeyIndex(rel) != RelationGetRelid(idxrel));
+
+ /*
+ * At this point, we are also sure that the index is not consisting
+ * of only expressions.
+ */
+#ifdef USE_ASSERT_CHECKING
+ indexInfo = BuildIndexInfo(idxrel);
+ Assert(!IndexOnlyOnExpression(indexInfo));
+#endif

I was a bit confused by the comment. IIUC the code has already called
the FilterOutNotSuitablePathsForReplIdentFull some point prior so all
the unwanted indexes are already filtered out. Therefore these
assertions are just for no reason, other than sanity checking that
fact, right? If my understand is correct perhaps a simpler single
comment is possible:

SUGGESTION (or something like this)
This attribute is an expression, however
FilterOutNotSuitablePathsForReplIdentFull was called earlier during
[...] and the indexes comprising only expressions have already been
eliminated. We sanity check this now. Furthermore, because primary key
and unique key indexes can't include expressions we also sanity check
the index is neither of those kinds.

~~~

9.
- return hasnulls;
+ /* We should always use at least one attribute for the index scan */
+ Assert (scankey_attoff > 0);

SUGGESTION
There should always be at least one attribute for the index scan.

~~~

10. src/backend/executor/execReplication.c - RelationFindReplTupleByIndex

ScanKeyData skey[INDEX_MAX_KEYS];
IndexScanDesc scan;
SnapshotData snap;
TransactionId xwait;
Relation idxrel;
bool found;
TypeCacheEntry **eq = NULL; /* only used when the index is not unique */
bool indisunique;
int scankey_attoff;

10a.
Should 'scankey_attoff' be called 'skey_attoff' for consistency with
the 'skey' array?

~

10b.
Also, it might be tidier to declare the 'skey_attoff' adjacent to the 'skey'.

======

11. src/backend/replication/logical/relation.c

For LogicalRepPartMap, I was wondering if it should keep a small
comment to xref back to the long comment which was moved to
logicalreplication.h

e.g.
/* Refer to the LogicalRepPartMapEntry comment in logicalrelation.h */

~~~

12. src/backend/replication/logical/relation.c - logicalrep_partition_open

+ /*
+ * Finding a usable index is an infrequent task. It occurs when
+ * an operation is first performed on the relation, or after
+ * invalidation of the relation cache entry (e.g., such as ANALYZE).
+ */
+ entry->usableIndexOid = FindLogicalRepUsableIndex(entry->localrel, remoterel);
  entry->localrelvalid = true;

Should there be a blank line between those assignments? (just for
consistency with the other code of this patch in a later function that
does exactly the same assignments).

~~~

13. src/backend/replication/logical/relation.c -
FilterOutNotSuitablePathsForReplIdentFull

Not sure about this function name. Maybe should be something like
'FilterOutUnsuitablePathsForReplIdentFull', or just
'SuitablePathsForReplIdentFull'

~~~

14.

+ else
+ {
+ Relation indexRelation;
+ IndexInfo *indexInfo;
+ bool is_btree;
+ bool is_partial;
+ bool is_only_on_expression;

Is that another var that could be renamed 'idxoid' like all the others?

~~~

15. src/backend/replication/logical/relation.c -
GetCheapestReplicaIdentityFullPath

+ typentry = lookup_type_cache(attr->atttypid,
+ TYPECACHE_EQ_OPR_FINFO);

Seems unnecessary wrapping.

~~~

15.

+ /*
+ * Currently it is not possible for planner to pick a
+ * partial index or indexes only on expressions. We
+ * still want to be explicit and eliminate such
+ * paths proactively.
...
...
+ */

This large comment seems unusually skinny. Needs pg_indent.

~~~

16. src/backend/replication/logical/worker.c - check_relation_updatable

@@ -1753,7 +1738,7 @@ check_relation_updatable(LogicalRepRelMapEntry *rel)
  * We are in error mode so it's fine this is somewhat slow. It's better to
  * give user correct error.
  */
- if (OidIsValid(GetRelationIdentityOrPK(rel->localrel)))
+ if (OidIsValid(rel->usableIndexOid))

The original comment about it being "somewhat slow" does not seem
relevant anymore because it is no longer calling a function in this
condition.

~~~

17. src/backend/replication/logical/worker.c - usable_indexoid_internal

+ relmapentry = &(part_entry->relmapentry);

The parentheses seem overkill, and code is not written like this
elsewhere in the same patch.

~~~

18. src/backend/replication/logical/worker.c - apply_handle_tuple_routing

@@ -2202,13 +2225,15 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
  * suitable partition.
  */
  {
+ LogicalRepRelMapEntry *entry = &part_entry->relmapentry;

I think elsewhere in the patch the same variable is called
'relmapentry' (which seems a bit better than just 'entry')

======

19. .../subscription/t/032_subscribe_use_index.pl

+# ANALYZING child will change the index used on child_1 and going to
use index_on_child_1_b
+$node_subscriber->safe_psql('postgres', "ANALYZE child_1");

19a.
"ANALYZING child" ? Should that be worded differently? There is
nothing named 'child' that I could see.

~

19b.
"and going to use" ? wording ? "which will be used for " ??

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Önder,

Thanks for updating the patch! I will check it later.
Currently I just reply to your comments.

> Hmm, I couldn't realize this comment earlier. So you suggest "slow" here refers to the additional function call
"GetRelationIdentityOrPK"?If so, yes I'll update that.
 

Yes I meant to say that, because functions will be called like:

GetRelationIdentityOrPK() -> RelationGetPrimaryKeyIndex() -> RelationGetIndexList() -> ..

and according to comments last one seems to do the heavy lifting.


> Makes sense, simplified the function. Though, it is always hard to pick good names for these kinds of helper
functions.I picked GenerateDummySelectPlannerInfoForRelation(), does that sound good to you as well?
 

I could not find any better naming than yours. 

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

I had quick look at the latest v11-0001 patch differences from v10.

Here are some initial comments:

======

1. Commit message

It looks like some small mistake happened. You wrote [1] that my
previous review comments about the commit message were fixed, but it
seems the v11 commit message is unchanged since v10.

======

2. src/backend/replication/logical/relation.c -
GenerateDummySelectPlannerInfoForRelation

+/*
+ * This is not a generic function, helper function for
+ * GetCheapestReplicaIdentityFullPath. The function creates
+ * a dummy PlannerInfo for the given relationId as if the
+ * relation is queried with SELECT command.
+ */
+static PlannerInfo *
+GenerateDummySelectPlannerInfoForRelation(Oid relationId)

"generic function, helper function" -> "generic function. It is a
helper function"


------
[1] https://www.postgresql.org/message-id/CACawEhXnTcXBOTofptkgSBOyD81Pohd7MSfFaW0SKo-0oKrCJg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

I've gone through the v11-0001 patch in more detail.

Here are some more review comments (nothing functional I think -
mostly just wording)

======

1. src/backend/executor/execReplication.c - build_replindex_scan_key

- * This is not generic routine, it expects the idxrel to be replication
- * identity of a rel and meet all limitations associated with that.
+ * This is not generic routine, it expects the idxrel to be an index
+ * that planner would choose if the searchslot includes all the columns
+ * (e.g., REPLICA IDENTITY FULL on the source).
  */
-static bool
+static int
 build_replindex_scan_key(ScanKey skey, Relation rel, Relation idxrel,
  TupleTableSlot *searchslot)


(I know this is not caused by your patch but maybe fix it at the same time?)

"This is not generic routine, it expects..." -> "This is not a generic
routine - it expects..."

~~~

2.

+ IndexInfo  *indexInfo PG_USED_FOR_ASSERTS_ONLY;
+
+ /*
+ * This attribute is an expression, and
+ * SuitablePathsForRepIdentFull() was called earlier while the
+ * index for subscriber is selected. There, the indexes comprising
+ * *only* expressions have already been eliminated.
+ *
+ * We sanity check this now.
+ */
+#ifdef USE_ASSERT_CHECKING
+ indexInfo = BuildIndexInfo(idxrel);
+ Assert(!IndexOnlyOnExpression(indexInfo));
+#endif

2a.
"while the index for subscriber is selected..." -> "when the index for
the subscriber was selected...”

~

2b.
Because there is only one declaration in this code block you could
simplify this a bit if you wanted to.

SUGGESTION
/*
 * This attribute is an expression, and
 * SuitablePathsForRepIdentFull() was called earlier while the
 * index for subscriber is selected. There, the indexes comprising
 * *only* expressions have already been eliminated.
 *
 * We sanity check this now.
 */
#ifdef USE_ASSERT_CHECKING
IndexInfo  *indexInfo = BuildIndexInfo(idxrel);
Assert(!IndexOnlyOnExpression(indexInfo));
#endif

~~~

3. src/backend/executor/execReplication.c - RelationFindReplTupleByIndex

+ /* Start an index scan. */
+ scan = index_beginscan(rel, idxrel, &snap, skey_attoff, 0);
 retry:
  found = false;

It might be better to have a blank line before that ‘retry’ label,
like in the original code.

======

4. src/backend/replication/logical/relation.c

+/* see LogicalRepPartMapEntry for details in logicalrelation.h */
 static HTAB *LogicalRepPartMap = NULL;

Personally, I'd word that something like:
"/* For LogicalRepPartMap details see LogicalRepPartMapEntry in
logicalrelation.h */"

but YMMV.

~~~

5. src/backend/replication/logical/relation.c -
GenerateDummySelectPlannerInfoForRelation

+/*
+ * This is not a generic function, helper function for
+ * GetCheapestReplicaIdentityFullPath. The function creates
+ * a dummy PlannerInfo for the given relationId as if the
+ * relation is queried with SELECT command.
+ */
+static PlannerInfo *
+GenerateDummySelectPlannerInfoForRelation(Oid relationId)

(mentioned this one in my previous post)

"This is not a generic function, helper function" -> "This is not a
generic function. It is a helper function"

~~~

6. src/backend/replication/logical/relation.c -
GetCheapestReplicaIdentityFullPath

+/*
+ * Generate all the possible paths for the given subscriber relation,
+ * for the cases that the source relation is replicated via REPLICA
+ * IDENTITY FULL. The function returns the cheapest Path among the
+ * eligible paths, see SuitablePathsForRepIdentFull().
+ *
+ * The function guarantees to return a path, because it adds sequential
+ * scan path if needed.
+ *
+ * The function assumes that all the columns will be provided during
+ * the execution phase, given that REPLICA IDENTITY FULL guarantees
+ * that.
+ */
+static Path *
+GetCheapestReplicaIdentityFullPath(Relation localrel)


"for the cases that..." -> "for cases where..."

~~~

7.

+ /*
+ * Currently it is not possible for planner to pick a partial index or
+ * indexes only on expressions. We still want to be explicit and eliminate
+ * such paths proactively.

"for planner..." -> "for the planner..."

======

8. .../subscription/t/032_subscribe_use_index.pl - general

8a.
(remove the 'we')
"# we wait until..." -> "# wait until..." X many occurrences

~

8b.
(remove the 'we')
"# we show that..." -> “# show that..." X many occurrences

~~~

9.

There is inconsistent wording for some of your test case start/end comments

9a.
e.g.
start: SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS
end: SUBSCRIPTION USES INDEX MODIFIES MULTIPLE ROWS

~

9b.
e.g.
start: SUBSCRIPTION USES INDEX WITH MULTIPLE COLUMNS
end: SUBSCRIPTION USES INDEX MODIFIES MULTIPLE ROWS

~~~

10.

I did not really understand the point of having special subscription names
tap_sub_rep_full_0
tap_sub_rep_full_2
tap_sub_rep_full_3
tap_sub_rep_full_4
etc...

Since you drop/recreate these for each test case can't they just be
called 'tap_sub_rep_full'?

~~~

11. SUBSCRIPTION USES INDEX WITH MULTIPLE COLUMNS

+# updates 200 rows
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM test_replica_id_full WHERE x IN (5, 6);");

The comment says update but this is doing delete


~~~

12. SUBSCRIPTION USES INDEX WITH DROPPED COLUMNS

+# cleanup sub
+$node_subscriber->safe_psql('postgres',
+ "DROP SUBSCRIPTION tap_sub_rep_full_4");

Unusual wrapping?

~~~

13. SUBSCRIPTION USES INDEX ON PARTITIONED TABLES

+# updates rows and moves between partitions
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM users_table_part WHERE user_id = 1 and value_1 = 1;");
+$node_publisher->safe_psql('postgres',
+ "DELETE FROM users_table_part WHERE user_id = 12 and value_1 = 12;");

The comment says update but SQL says delete

~~~

14. SUBSCRIPTION CAN USE INDEXES WITH EXPRESSIONS AND COLUMNS

+# update 1 row and delete 1 row using index_b, so index_a still has 2 idx_scan
+$node_subscriber->poll_query_until(
+ 'postgres', q{select idx_scan=2 from pg_stat_all_indexes where
indexrelname = 'index_a';}
+) or die "Timed out while waiting for check subscriber
tap_sub_rep_full_0 updates two rows via index scan with index on high
cardinality column-3";
+

The comment seems misplaced. Doesn't it belong on the lines above this
where the update/delete is being done?

~~~

15. SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE - INHERITED TABLE

+# ANALYZING child will change the index used on child_1 and going to
use index_on_child_1_b
+$node_subscriber->safe_psql('postgres', "ANALYZE child_1");

Should the comment say 'child_1' instead of child?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Hi Onder,

Thanks for addressing all my previous feedback. I checked the latest
v12-0001, and have no more comments at this time.

One last thing - do you think there is any need to mention this
behaviour in the pgdocs, or is OK just to be a hidden performance
improvement?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

> One last thing - do you think there is any need to mention this
> behaviour in the pgdocs, or is OK just to be a hidden performance
> improvement?

FYI - I put my opinion.
We have following sentence in the logical-replication.sgml:

```
...
If the table does not have any suitable key, then it can be set
   to replica identity <quote>full</quote>, which means the entire row becomes
   the key.  This, however, is very inefficient and should only be used as a
   fallback if no other solution is possible.
...
```

Here the word "very inefficient" may mean that sequential scans will be executed every time.
I think some descriptions can be added around here.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Tues, Sep 20, 2022 at 18:30 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> Thanks for the reviews, attached v12.

Thanks for your patch. Here is a question and a comment:

1. In the function GetCheapestReplicaIdentityFullPath.
+    if (rel->pathlist == NIL)
+    {
+        /*
+         * A sequential scan could have been dominated by by an index scan
+         * during make_one_rel(). We should always have a sequential scan
+         * before set_cheapest().
+         */
+        Path       *seqScanPath = create_seqscan_path(root, rel, NULL, 0);
+
+        add_path(rel, seqScanPath);
+    }

This is a question I'm not sure about:
Do we need this part to add sequential scan?

I think in our case, the sequential scan seems to have been added by the
function make_one_rel (see function set_plain_rel_pathlist). If I am missing
something, please let me know. BTW, there is a typo in above comment: `by by`.

2. In the file execReplication.c.
+#ifdef USE_ASSERT_CHECKING
+#include "catalog/index.h"
+#endif
 #include "commands/trigger.h"
 #include "executor/executor.h"
 #include "executor/nodeModifyTable.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parse_relation.h"
 #include "parser/parsetree.h"
+#ifdef USE_ASSERT_CHECKING
+#include "replication/logicalrelation.h"
+#endif

I think it's fine to only add `logicalrelation.h` here, because `index.h` has
been added by `logicalrelation.h`.

Regards,
Wang wei

On Thu, Sep 22, 2022 at 9:44 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
> Also, do you think is this a good time for me to mark the patch "Ready for committer" in the commit fest? Not sure
whenand who should change the state, but it seems I can change. I couldn't find any documentation on how that process
shouldwork. 
>

Normally, the reviewers mark it as "Ready for committer".

--
With Regards,
Amit Kapila.

Dear Önder:

Thank you for updating patch! 
Your documentation seems OK, and I could not find any other places to be added

Followings are my comments.

====
01 relation.c - general

Many files are newly included.
I was not sure but some codes related with planner may be able to move to src/backend/optimizer/plan.
How do you and any other one think?

02 relation.c - FindLogicalRepUsableIndex

```
+/*
+ * Returns an index oid if we can use an index for the apply side. If not,
+ * returns InvalidOid.
+ */
+static Oid
+FindLogicalRepUsableIndex(Relation localrel, LogicalRepRelation *remoterel)
```

I grepped files, but I cannot find the word "apply side". How about "subscriber" instead?

03 relation.c - FindLogicalRepUsableIndex

```
+       /* Simple case, we already have an identity or pkey */
+       idxoid = GetRelationIdentityOrPK(localrel);
+       if (OidIsValid(idxoid))
+               return idxoid;
+
+       /*
+        * If index scans are disabled, use a sequential scan.
+        *
+        * Note that we still allowed index scans above when there is a primary
+        * key or unique index replica identity, but that is the legacy behaviour
+        * (even when enable_indexscan is false), so we hesitate to move this
+        * enable_indexscan check to be done earlier in this function.
+        */
+       if (!enable_indexscan)
+               return InvalidOid;
```

a. 
I think "identity or pkey" should be "replica identity key or primary key" or "RI or PK"

b. 
Later part should be at around GetRelationIdentityOrPK.


04 relation.c - FindUsableIndexForReplicaIdentityFull

```
+       MemoryContext usableIndexContext;
...
+       usableIndexContext = AllocSetContextCreate(CurrentMemoryContext,
+                                                                                          "usableIndexContext",
+                                                                                          ALLOCSET_DEFAULT_SIZES);
```

I grepped other sources, and I found that the name like "tmpcxt" is used for the temporary MemoryContext.

05 relation.c - SuitablePathsForRepIdentFull

```
+                       indexRelation = index_open(idxoid, AccessShareLock);
+                       indexInfo = BuildIndexInfo(indexRelation);
+                       is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
+                       is_partial = (indexInfo->ii_Predicate != NIL);
+                       is_only_on_expression = IndexOnlyOnExpression(indexInfo);
+                       index_close(indexRelation, NoLock);
```

Why the index is closed with NoLock? AccessShareLock is acquired, so shouldn't same lock be released?


06 relation.c - GetCheapestReplicaIdentityFullPath

IIUC a query like "SELECT tbl WHERE attr1 = $1 AND attr2 = $2 ... AND attrN = $N" is emulated, right?
you can write explicitly it as comment

07 relation.c - GetCheapestReplicaIdentityFullPath

```
+               Path       *path = (Path *) lfirst(lc);
+               Oid                     idxoid = GetIndexOidFromPath(path);
+
+               if (!OidIsValid(idxoid))
+               {
+                       /* Unrelated Path, skip */
+                       suitableIndexList = lappend(suitableIndexList, path);
+               }
```

I was not clear about here. IIUC in the function we want to extract "good" scan plan and based on that the cheapest one
ischosen. 
 
GetIndexOidFromPath() seems to return InvalidOid when the input path is not index scan, so why is it appended to the
suitablelist?
 


===
08 worker.c - usable_indexoid_internal

I think this is not "internal" function, such name should be used for like "apply_handle_commit" -
"apply_handle_commit_internal",or "apply_handle_insert" - "apply_handle_insert_internal".
 
How about "get_usable_index" or something?

09 worker.c - usable_indexoid_internal

```
+       Oid                     targetrelid = targetResultRelInfo->ri_RelationDesc->rd_rel->oid;
+       Oid                     localrelid = relinfo->ri_RelationDesc->rd_id;
+
+       if (targetrelid != localrelid)
```

I think these lines are very confusable.
IIUC targetrelid is corresponded to the "parent", and localrelid is corresponded to the "child", right?
How about changing name to "partitionedoid" and "leadoid" or something?

===
10 032_subscribe_use_index.pl

```
# create tables pub and sub
$node_publisher->safe_psql('postgres',
    "CREATE TABLE test_replica_id_full (x int)");
$node_publisher->safe_psql('postgres',
    "ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;");
$node_subscriber->safe_psql('postgres',
    "CREATE TABLE test_replica_id_full (x int)");
$node_subscriber->safe_psql('postgres',
    "CREATE INDEX test_replica_id_full_idx ON test_replica_id_full(x)");
```

In many places same table is defined, altered as "REPLICA IDENTITY FULL", and index is created.
Could you combine them into function?

11 032_subscribe_use_index.pl

```
# wait until the index is used on the subscriber
$node_subscriber->poll_query_until(
    'postgres', q{select (idx_scan = 1) from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full_0 updates one row via index";
```

In many places this check is done. Could you combine them into function?

12 032_subscribe_use_index.pl

```
# create pub/sub
$node_publisher->safe_psql('postgres',
    "CREATE PUBLICATION tap_pub_rep_full FOR TABLE test_replica_id_full");
$node_subscriber->safe_psql('postgres',
    "CREATE SUBSCRIPTION tap_sub_rep_full CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION
tap_pub_rep_full"
);
```

Same as above

13 032_subscribe_use_index.pl

```
# cleanup pub
$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_rep_full");
$node_publisher->safe_psql('postgres', "DROP TABLE test_replica_id_full");
# cleanup sub
$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub_rep_full");
$node_subscriber->safe_psql('postgres', "DROP TABLE test_replica_id_full");
```

Same as above

14 032_subscribe_use_index.pl - SUBSCRIPTION USES INDEX

```
# make sure that the subscriber has the correct data
my $result = $node_subscriber->safe_psql('postgres',
    "SELECT sum(x) FROM test_replica_id_full");
is($result, qq(212), 'ensure subscriber has the correct data at the end of the test');

$node_subscriber->poll_query_until(
    'postgres', q{select sum(x)=212 AND count(*)=21 AND count(DISTINCT x)=20 FROM test_replica_id_full;}
) or die "ensure subscriber has the correct data at the end of the test";
```

I think first one is not needed.


15 032_subscribe_use_index.pl - SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS

```
# insert some initial data within the range 0-20
$node_publisher->safe_psql('postgres',
    "INSERT INTO test_replica_id_full SELECT i%20 FROM generate_series(0,1000)i;"
);
```

I think data is within the range 0-19.
(There are some mistakes)

===
16 test/subscription/meson.build

Your test 't/032_subscribe_use_index.pl' must be added in the 'tests' for meson build system.
(I checked on my env, and your test works well)

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Önder,

Thank you for updating the patch! At first I replied to your comments.

> My thinking on those functions is that they should probably stay
> in src/backend/replication/logical/relation.c. My main motivation is that
> those functions are so much tailored to the purposes of this file that I
> cannot see any use-case for these functions in any other context.

I was not sure what should be, but I agreed that functions will be not used from other parts.

> Hmm, I cannot follow this comment. Can you please clarify?

In your patch:

```
+       /* Simple case, we already have a primary key or a replica identity index */
+       idxoid = GetRelationIdentityOrPK(localrel);
+       if (OidIsValid(idxoid))
+               return idxoid;
+
+       /*
+        * If index scans are disabled, use a sequential scan.
+        *
+        * Note that we still allowed index scans above when there is a primary
+        * key or unique index replica identity, but that is the legacy behaviour
+        * (even when enable_indexscan is false), so we hesitate to move this
+        * enable_indexscan check to be done earlier in this function.
+        */ 
```

And the paragraph " Note that we..." should be at above of GetRelationIdentityOrPK().
Future readers will read the function from top to bottom,
and when they read around GetRelationIdentityOrPK() they may be confused.

> So, I think it is better to have specific names, no?

OK.

> The inlined comment in the function has a similar comment. Is that clear
> enough?
> /* * Generate restrictions for all columns in the form of col_1 = $1 AND *
> col_2 = $2 ... */

Actually I missed it, but I still think that whole of emulated SQL should be clarified. 

> Though, I agree that we can improve the code a bit. I now
> use targetrelkind and dropped localrelid to check whether the target is a
> partitioned table. Is this better?

Great improvement. Genus!

> Well, I'm not sure if it is worth the complexity. There are only 4 usages
> of the same table, and these are all pretty simple statements, and all
> other tests seem to have a similar pattern. I have not seen any tests where
> these simple statements are done in a function even if they are repeated.
> I'd rather keep it so that this doesn't lead to other style discussions?

If other tests do not combine such parts, it's OK.
My motivation of these comments were to reduce the number of row for the test code.

> Oh, I didn't know about this, thanks!

Now meson test system do your test. OK.


And followings are the comments for v14. They are mainly about comments.

===
01. relation.c - logicalrep_rel_open

```
+               /*
+                * Finding a usable index is an infrequent task. It occurs when an
+                * operation is first performed on the relation, or after invalidation
+                * of the relation cache entry (e.g., such as ANALYZE).
+                */
+               entry->usableIndexOid = FindLogicalRepUsableIndex(entry->localrel, remoterel);
```

I thought you can mention CREATE INDEX in the comment.

According to your analysis [1] the relation cache will be invalidated if users do CREATE INDEX
At that time the hash entry will be removed (logicalrep_relmap_invalidate_cb) and "usable" index
will be checked again.

~~~
02. relation.c - logicalrep_partition_open

```
+       /*
+        * Finding a usable index is an infrequent task. It occurs when an
+        * operation is first performed on the relation, or after invalidation of
+        * the relation cache entry (e.g., such as ANALYZE).
+        */
+       entry->usableIndexOid = FindLogicalRepUsableIndex(partrel, remoterel);
+
```

Same as above

~~~
03. relation.c - GetIndexOidFromPath

```
+       if (path->pathtype == T_IndexScan || path->pathtype == T_IndexOnlyScan)
+       {
+               IndexPath  *index_sc = (IndexPath *) path;
+
+               return index_sc->indexinfo->indexoid;
+       }
```

I thought Assert(OidIsValid(indexoid)) may be added here. Or is it quite trivial?

~~~
04. relation.c - IndexOnlyOnExpression

This method just returns "yes" or "no", so the name of method should be start "Has" or "Is".

~~~
05. relation.c - SuitablePathsForRepIdentFull

```
+/*
+ * Iterates over the input path list and returns another
+ * path list that includes index [only] scans where paths
+ * with non-btree indexes, partial indexes or
+ * indexes on only expressions have been removed.
+ */
```

These lines seems to be around 60 columns. Could you expand around 80?

~~~
06. relation.c - SuitablePathsForRepIdentFull

```
+                       Relation        indexRelation;
+                       IndexInfo  *indexInfo;
+                       bool            is_btree;
+                       bool            is_partial;
+                       bool            is_only_on_expression;
+
+                       indexRelation = index_open(idxoid, AccessShareLock);
+                       indexInfo = BuildIndexInfo(indexRelation);
+                       is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
+                       is_partial = (indexInfo->ii_Predicate != NIL);
+                       is_only_on_expression = IndexOnlyOnExpression(indexInfo);
+                       index_close(indexRelation, AccessShareLock);
+
+                       if (is_btree && !is_partial && !is_only_on_expression)
+                               suitableIndexList = lappend(suitableIndexList, path);
```

Please add a comment like "eliminating not suitable path" or something.

~~~
07. relation.c - GenerateDummySelectPlannerInfoForRelation

```
+/*
+ * This is not a generic function. It is a helper function
+ * for GetCheapestReplicaIdentityFullPath. The function
+ * creates a dummy PlannerInfo for the given relationId
+ * as if the relation is queried with SELECT command.
+ */
```

These lines seems to be around 60 columns. Could you expand around 80?

~~~
08. relation.c - FindLogicalRepUsableIndex

```
+/*
+ * Returns an index oid if we can use an index for subscriber . If not,
+ * returns InvalidOid.
+ */
```

"subscriber ." should be "subscriber.", blank is not needed.

~~~
09. worker.c - get_usable_indexoid

```
+               Assert(targetResultRelInfo->ri_RelationDesc->rd_rel->relkind ==
+                          RELKIND_PARTITIONED_TABLE);
```

I thought this assertion seems to be not needed, because this is completely same as the condition of if-statement.

[1] https://www.postgresql.org/message-id/CACawEhXgP_Kj_1iyNAp16MYos4Anrtz%2BOZVtj2z-QOPGdPCt_A%40mail.gmail.com


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Önder,

Thanks for updating the patch! I checked yours and almost good.
Followings are just cosmetic comments.

===
01. relation.c - GetCheapestReplicaIdentityFullPath

```
     * The reason that the planner would not pick partial indexes and indexes
     * with only expressions based on the way currently baserestrictinfos are
     * formed (e.g., col_1 = $1 ... AND col_N = $2).
```

Is "col_N = $2" a typo? I think it should be "col_N = $N" or "attr1 = $1 ... AND attrN = $N".

===
02. 032_subscribe_use_index.pl

If a table has a primary key on the subscriber, it will be used even if enable_indexscan is false(legacy behavior).
Should we test it?

~~~
03. 032_subscribe_use_index.pl -  SUBSCRIPTION RE-CALCULATES INDEX AFTER CREATE/DROP INDEX

I think this test seems to be not trivial, so could you write down the motivation?

~~~
04. 032_subscribe_use_index.pl -  SUBSCRIPTION RE-CALCULATES INDEX AFTER CREATE/DROP INDEX

```
# wait until the index is created
$node_subscriber->poll_query_until(
    'postgres', q{select count(*)=1 from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full_0 updates one row via index";
```

CREATE INDEX is a synchronous behavior, right? If so we don't have to wait here.
...And the comment in case of die may be wrong.
(There are some cases like this)

~~~
05. 032_subscribe_use_index.pl - SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS

```
# Testcase start: SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS
#
# Basic test where the subscriber uses index
# and touches 50 rows with UPDATE
```

"touches 50 rows with UPDATE" -> "updates 50 rows", per other tests.

~~~
06. 032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE

I think this test seems to be not trivial, so could you write down the motivation?
(Same as Re-calclate)

~~~
07. 032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE

```
# show that index_b is not used
$node_subscriber->poll_query_until(
    'postgres', q{select idx_scan=0 from pg_stat_all_indexes where indexrelname = 'index_b';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on
highcardinality column-2";
 
```

I think we don't have to wait here, is() should be used instead. 
poll_query_until() should be used only when idx_scan>0 is checked.
(There are some cases like this)

~~~
08. 032_subscribe_use_index.pl - SUBSCRIPTION USES INDEX ON PARTITIONED TABLES

```
# make sure that the subscriber has the correct data
$node_subscriber->poll_query_until(
    'postgres', q{select sum(user_id+value_1+value_2)=550070 AND count(DISTINCT(user_id,value_1, value_2))=981 from
users_table_part;}
) or die "ensure subscriber has the correct data at the end of the test";
```

I think we can replace it to wait_for_catchup() and is()...
Moreover, we don't have to wait here because in above line we wait until the index is used on the subscriber.
(There are some cases like this)


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Önder,

Thank you for updating the patch!

> It is not about CREATE INDEX being async. It is about pg_stat_all_indexes
> being async. If we do not wait, the tests become flaky, because sometimes
> the update has not been reflected in the view immediately.

Make sense, I forgot how stats collector works...

Followings are comments for v16. Only for test codes.

~~~
01. 032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE

```
# show that index_b is not used
$node_subscriber->poll_query_until(
    'postgres', q{select idx_scan=0 from pg_stat_all_indexes where indexrelname = 'index_b';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on
highcardinality column-2";
 
```

poll_query_until() is still remained here, it should be replaced to is().


02. 032_subscribe_use_index.pl - SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

```
# show that the unique index on replica identity is used even when enable_indexscan=false
$result = $node_subscriber->safe_psql('postgres',
    "select idx_scan from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx'");
is($result, qq(0), 'ensure subscriber has not used index with enable_indexscan=false');
```

Is the comment wrong? The index test_replica_id_full_idx is not used here.


032_subscribe_use_index.pl - SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE

```
# show that index_b is not used
$node_subscriber->poll_query_until(
    'postgres', q{select idx_scan=0 from pg_stat_all_indexes where indexrelname = 'index_b';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on
highcardinality column-2";
 
```

poll_query_until() is still remained here, it should be replaced to is()

032_subscribe_use_index.pl - SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

```
# show that the unique index on replica identity is used even when enable_indexscan=false
$result = $node_subscriber->safe_psql('postgres',
    "select idx_scan from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx'");
is($result, qq(0), 'ensure subscriber has not used index with enable_indexscan=false');
```

Is the comment wrong? The index test_replica_id_full_idx is not used here.

03. 032_subscribe_use_index.pl - SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

```
$node_publisher->safe_psql('postgres',
    "ALTER TABLE test_replica_id_full REPLICA IDENTITY USING INDEX test_replica_id_full_unique;");
```

I was not sure why ALTER TABLE REPLICA IDENTITY USING INDEX was done on the publisher side.
IIUC this feature works when REPLICA IDENTITY FULL is specified on a publisher,
so it might not be altered here. If so, an index does not have to define on the publisher too.

04. 032_subscribe_use_index.pl - SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN

```
$node_subscriber->poll_query_until(
    'postgres', q{select (idx_scan=1) from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_unique'}
) or die "Timed out while waiting ensuring subscriber used unique index as replica identity even with
enable_indexscan=false'";
```

03 comment should be added here.


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Önder,

Thanks for updating the patch!

I think your saying seems reasonable.
I have no comments anymore now. Thanks for updating so quickly.


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Aug 24, 2022 12:25 AM Önder Kalacı <onderkalaci@gmail.com> wrote:
> Hi,
> 
> Thanks for the review!
> 

Thanks for your reply.

> 
> >
> > 1.
> > In FilterOutNotSuitablePathsForReplIdentFull(), is
> > "nonPartialIndexPathList" a
> > good name for the list? Indexes on only expressions are also be filtered.
> >
> > +static List *
> > +FilterOutNotSuitablePathsForReplIdentFull(List *pathlist)
> > +{
> > +       ListCell   *lc;
> > +       List *nonPartialIndexPathList = NIL;
> >
> >
> Yes, true. We only started filtering the non-partial ones first. Now
> changed to *suitableIndexList*, does that look right?
> 

That looks ok to me.

> 
> 
> > 3.
> > It looks we should change the comment for FindReplTupleInLocalRel() in this
> > patch.
> >
> > /*
> >  * Try to find a tuple received from the publication side (in
> > 'remoteslot') in
> >  * the corresponding local relation using either replica identity index,
> >  * primary key or if needed, sequential scan.
> >  *
> >  * Local tuple, if found, is returned in '*localslot'.
> >  */
> > static bool
> > FindReplTupleInLocalRel(EState *estate, Relation localrel,
> >
> >
> I made a small change, just adding "index". Do you expect a larger change?
> 
> 

I think that's sufficient.

> 
> 
> > 5.
> > +               if (!AttributeNumberIsValid(mainattno))
> > +               {
> > +                       /*
> > +                        * There are two cases to consider. First, if the
> > index is a primary or
> > +                        * unique key, we cannot have any indexes with
> > expressions. So, at this
> > +                        * point we are sure that the index we deal is not
> > these.
> > +                        */
> > +                       Assert(RelationGetReplicaIndex(rel) !=
> > RelationGetRelid(idxrel) &&
> > +                                  RelationGetPrimaryKeyIndex(rel) !=
> > RelationGetRelid(idxrel));
> > +
> > +                       /*
> > +                        * For a non-primary/unique index with an
> > expression, we are sure that
> > +                        * the expression cannot be used for replication
> > index search. The
> > +                        * reason is that we create relevant index paths
> > by providing column
> > +                        * equalities. And, the planner does not pick
> > expression indexes via
> > +                        * column equality restrictions in the query.
> > +                        */
> > +                       continue;
> > +               }
> >
> > Is it possible that it is a usable index with an expression? I think
> > indexes
> > with an expression has been filtered in
> > FilterOutNotSuitablePathsForReplIdentFull(). If it can't be a usable index
> > with
> > an expression, maybe we shouldn't use "continue" here.
> >
> 
> 
> 
> Ok, I think there are some confusing comments in the code, which I updated.
> Also, added one more explicit Assert to make the code a little more
> readable.
> 
> We can support indexes involving expressions but not indexes that are only
> consisting of expressions. FilterOutNotSuitablePathsForReplIdentFull()
> filters out the latter, see IndexOnlyOnExpression().
> 
> So, for example, if we have an index as below, we are skipping the
> expression while building the index scan keys:
> 
> CREATE INDEX people_names ON people (firstname, lastname, (id || '_' ||
> sub_id));
> 
> We can consider removing `continue`, but that'd mean we should also adjust
> the following code-block to handle indexprs. To me, that seems like an edge
> case to implement at this point, given such an index is probably not
> common. Do you think should I try to use the indexprs as well while
> building the scan key?
> 
> I'm mostly trying to keep the complexity small. If you suggest this
> limitation should be lifted, I can give it a shot. I think the limitation I
> leave here is with a single sentence: *The index on the subscriber can only
> use simple column references.  *
> 

Thanks for your explanation. I get it and think it's OK.

> > 6.
> > In the following case, I got a result which is different from HEAD, could
> > you
> > please look into it?
> >
> > -- publisher
> > CREATE TABLE test_replica_id_full (x int);
> > ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;
> > CREATE PUBLICATION tap_pub_rep_full FOR TABLE test_replica_id_full;
> >
> > -- subscriber
> > CREATE TABLE test_replica_id_full (x int, y int);
> > CREATE INDEX test_replica_id_full_idx ON test_replica_id_full(x,y);
> > CREATE SUBSCRIPTION tap_sub_rep_full_0 CONNECTION 'dbname=postgres
> > port=5432' PUBLICATION tap_pub_rep_full;
> >
> > -- publisher
> > INSERT INTO test_replica_id_full VALUES (1);
> > UPDATE test_replica_id_full SET x = x + 1 WHERE x = 1;
> >
> > The data in subscriber:
> > on HEAD:
> > postgres=# select * from test_replica_id_full ;
> >  x | y
> > ---+---
> >  2 |
> > (1 row)
> >
> > After applying the patch:
> > postgres=# select * from test_replica_id_full ;
> >  x | y
> > ---+---
> >  1 |
> > (1 row)
> >
> >
> Ops, good catch. it seems we forgot to have:
> 
> skey[scankey_attoff].sk_flags |= SK_SEARCHNULL;
> 
> On head, the index used for this purpose could only be the primary key or
> unique key on NOT NULL columns. Now,  we do allow NULL values, and need to
> search for them. Added that (and your test) to the updated patch.
> 
> As a semi-related note, tuples_equal() decides `true` for (NULL = NULL). I
> have not changed that, and it seems right in this context. Do you see any
> issues with that?
> 
> Also, I realized that the functions in the execReplication.c expect only
> btree indexes. So, I skipped others as well. If that makes sense, I can
> work on a follow-up patch after we can merge this, to remove some of the
> limitations mentioned here.

Thanks for fixing it and updating the patch, I didn't see any issue about it.

Here are some comments on v17 patch.

1. 
-LogicalRepRelMapEntry *
+LogicalRepPartMapEntry *
 logicalrep_partition_open(LogicalRepRelMapEntry *root,
                           Relation partrel, AttrMap *map)
 {

Is there any reason to change the return type of logicalrep_partition_open()? It
seems ok without this change.

2. 

+         * of the relation cache entry (e.g., such as ANALYZE or
+         * CREATE/DROP index on the relation).

"e.g." and "such as" mean the same. I think we remove one of them.

3.
+$node_subscriber->poll_query_until(
+    'postgres', q{select (idx_scan = 2) from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';}
+) or die "Timed out while waiting for'check subscriber tap_sub_rep_full deletes one row via index";
+

+$node_subscriber->poll_query_until(
+    'postgres', q{select (idx_scan = 1) from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idy';}
+) or die "Timed out while waiting for'check subscriber tap_sub_rep_full deletes one row via index";


"for'check" -> "for check"

3.
+$node_subscriber->safe_psql('postgres',
+    "SELECT pg_reload_conf();");
+
+# Testcase start: SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN
+# ====================================================================
+
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
+

"Testcase start" in the comment should be "Testcase end".

4.
There seems to be a problem in the following scenario, which results in
inconsistent data between publisher and subscriber.

-- publisher
CREATE TABLE test_replica_id_full (x int, y int);
ALTER TABLE test_replica_id_full REPLICA IDENTITY FULL;
CREATE PUBLICATION tap_pub_rep_full FOR TABLE test_replica_id_full;

-- subscriber
CREATE TABLE test_replica_id_full (x int, y int);
CREATE UNIQUE INDEX test_replica_id_full_idx ON test_replica_id_full(x);
CREATE SUBSCRIPTION tap_sub_rep_full_0 CONNECTION 'dbname=postgres port=5432' PUBLICATION tap_pub_rep_full;

-- publisher
INSERT INTO test_replica_id_full VALUES (NULL,1);
INSERT INTO test_replica_id_full VALUES (NULL,2);
INSERT INTO test_replica_id_full VALUES (NULL,3);
update test_replica_id_full SET x=1 where y=2;

The data in publisher:
postgres=# select * from test_replica_id_full order by y;
 x | y
---+---
   | 1
 1 | 2
   | 3
(3 rows)

The data in subscriber:
postgres=# select * from test_replica_id_full order by y;
 x | y
---+---
   | 2
 1 | 2
   | 3
(3 rows)

There is no such problem on master branch.


Regards,
Shi yu

On Fri, Sep 23, 2022 at 0:14 AM Önder Kalacı <onderkalaci@gmail.com> wrote:
> Hii Wang wei,

Thanks for updating the patch and your reply.

> > 1. In the function GetCheapestReplicaIdentityFullPath.
> > +    if (rel->pathlist == NIL)
> > +    {
> > +        /*
> > +         * A sequential scan could have been dominated by by an index
> > scan
> > +         * during make_one_rel(). We should always have a sequential
> > scan
> > +         * before set_cheapest().
> > +         */
> > +        Path       *seqScanPath = create_seqscan_path(root, rel, NULL,
> > 0);
> > +
> > +        add_path(rel, seqScanPath);
> > +    }
> >
> > This is a question I'm not sure about:
> > Do we need this part to add sequential scan?
> >
> > I think in our case, the sequential scan seems to have been added by the
> > function make_one_rel (see function set_plain_rel_pathlist).
> 
> Yes, the sequential scan is added during make_one_rel.
> 
> > If I am missing something, please let me know. BTW, there is a typo in
> > above comment: `by by`.
> 
> As the comment mentions, the sequential scan could have been dominated &
> removed by index scan, see add_path():
> 
> *We also remove from the rel's pathlist any old paths that are dominated
> *  by new_path --- that is, new_path is cheaper, at least as well ordered,
> *  generates no more rows, requires no outer rels not required by the old
> *  path, and is no less parallel-safe.
> 
> Still, I agree that the comment could be improved, which I pushed.

Oh, sorry I didn't realize this part of the logic. Thanks for sharing this.

And I have another confusion about function GetCheapestReplicaIdentityFullPath:
If rel->pathlist is NIL, could we return NULL directly from this function, and
then set idxoid to InvalidOid in function FindUsableIndexForReplicaIdentityFull
in that case?

===

Here are some comments for test file  032_subscribe_use_index.pl on v18 patch:

1.
```
+# Basic test where the subscriber uses index
+# and only updates 1 row for and deletes
+# 1 other row
```
There seems to be an extra "for" here.

2. Typos for subscription name in the error messages.
tap_sub_rep_full_0 -> tap_sub_rep_full

3. Typo in comments
```
+# use the newly created index (provided that it fullfils the requirements).
```
fullfils -> fulfils

4. Some extra single quotes at the end of the error message ('").
For example:
```
# wait until the index is used on the subscriber
$node_subscriber->poll_query_until(
    'postgres', q{select (idx_scan = 200) from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates 200 rows via index'";
```

5. The column names in the error message appear to be a typo.
```
+) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on
highcardinality column-1";
 
...
+) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on
highcardinality column-3";
 
...
+) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates two rows via index scan with index on
highcardinality column-4";
 
```
It seems that we need to do the following change: 'column-3' -> 'column-1' and
'column-4' -> 'column-2'.
Or we could use the column names directly like this: 'column-1' -> 'column a',
'column_3' -> 'column a' and 'column_4' -> 'column b'.

6. DELETE action is missing from the error message.
```
+# 2 rows from first command, another 2 from the second command
+# overall index_on_child_1_a is used 4 times
+$node_subscriber->poll_query_until(
+    'postgres', q{select idx_scan=4 from pg_stat_all_indexes where indexrelname = 'index_on_child_1_a';}
+) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates child_1 table'";
```
I think we execute both UPDATE and DELETE for child_1 here. Could we add DELETE
action to this error message?

7. Table name in the error message.
```
# check if the index is used even when the index has NULL values
$node_subscriber->poll_query_until(
    'postgres', q{select idx_scan=2 from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';}
) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates parent table'";
```
It seems to be "test_replica_id_full" here instead of "parent'".

Regards,
Wang wei

On Wed, Oct 19, 2022 12:05 AM Önder Kalacı <onderkalaci@gmail.com>  wrote:
> 
> Attached v19.
> 

Thanks for updating the patch. Here are some comments on v19.

1.
In execReplication.c:

+    TypeCacheEntry **eq = NULL; /* only used when the index is not unique */

Maybe the comment here should be changed. Now it is used when the index is not
primary key or replica identity index.

2.
+# wait until the index is created
+$node_subscriber->poll_query_until(
+    'postgres', q{select count(*)=1 from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';}
+) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates one row via index";

The message doesn't seem right,  should it be changed to "Timed out while
waiting for creating index test_replica_id_full_idx"?

3.
+# now, ingest more data and create index on column y which has higher cardinality
+# then create an index on column y so that future commands uses the index on column
+$node_publisher->safe_psql('postgres',
+    "INSERT INTO test_replica_id_full SELECT 50, i FROM generate_series(0,3100)i;");

The comment say "create (an) index on column y" twice, maybe it can be changed
to:

now, ingest more data and create index on column y which has higher cardinality,
so that future commands will use the index on column y

4.
+# deletes 200 rows
+$node_publisher->safe_psql('postgres',
+    "DELETE FROM test_replica_id_full WHERE x IN (5, 6);");
+
+# wait until the index is used on the subscriber
+$node_subscriber->poll_query_until(
+    'postgres', q{select (idx_scan = 200) from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx';}
+) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates 200 rows via index";

It would be better to call wait_for_catchup() after DELETE. (And some other
places in this file.)
Besides, the "updates" in the message should be "deletes".

5.
+# wait until the index is used on the subscriber
+$node_subscriber->poll_query_until(
+    'postgres', q{select sum(idx_scan)=10 from pg_stat_all_indexes where indexrelname ilike 'users_table_part_%';}
+) or die "Timed out while waiting for check subscriber tap_sub_rep_full updates partitioned table";

Maybe we should say "updates partitioned table with index" in this message.

Regards,
Shi yu

Hi,

On 2022-11-11 17:16:36 +0100, Önder Kalacı wrote:
> I rebased the changes to the current master branch, reflected pg_indent
> suggestions and also made a few minor style changes.

Needs another rebase, I think:

https://cirrus-ci.com/task/5592444637544448

[05:44:22.102] FAILED: src/backend/postgres_lib.a.p/replication_logical_worker.c.o 
[05:44:22.102] ccache cc -Isrc/backend/postgres_lib.a.p -Isrc/include -I../src/include -Isrc/include/storage
-Isrc/include/utils-Isrc/include/catalog -Isrc/include/nodes -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64
-Wall-Winvalid-pch -g -fno-strict-aliasing -fwrapv -fexcess-precision=standard -D_GNU_SOURCE -Wmissing-prototypes
-Wpointer-arith-Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wshadow=compatible-local-Wformat-security -Wdeclaration-after-statement -Wno-format-truncation
-Wno-stringop-truncation-fPIC -pthread -DBUILDING_DLL -MD -MQ
src/backend/postgres_lib.a.p/replication_logical_worker.c.o-MF
src/backend/postgres_lib.a.p/replication_logical_worker.c.o.d-o
src/backend/postgres_lib.a.p/replication_logical_worker.c.o-c ../src/backend/replication/logical/worker.c
 
[05:44:22.102] ../src/backend/replication/logical/worker.c: In function ‘get_usable_indexoid’:
[05:44:22.102] ../src/backend/replication/logical/worker.c:2101:36: error: ‘ResultRelInfo’ has no member named
‘ri_RootToPartitionMap’
[05:44:22.102]  2101 |   TupleConversionMap *map = relinfo->ri_RootToPartitionMap;
[05:44:22.102]       |                                    ^~

Greetings,

Andres Freund

=?UTF-8?B?w5ZuZGVyIEthbGFjxLE=?= <onderkalaci@gmail.com> writes:
> Attached v22.

I took a very brief look through this.  I'm not too pleased with
this whole line of development TBH.  It seems to me that the core
design of execReplication.c and related code is "let's build our
own half-baked executor and much-less-than-half-baked planner,
because XXX".  (I'm not too sure what XXX was, really, but apparently
somebody managed to convince people that that is a sane and
maintainable design.)  Now this patch has decided that it *will*
use the real planner, or at least portions of it in some cases.
If we're going to do that ISTM we ought to replace all the existing
not-really-a-planner logic, but this has not done that; instead
we have a large net addition to the already very duplicative
replication code, with weird restrictions because it doesn't want
to make changes to the half-baked executor.

I think we should either live within the constraints set by this
overarching design, or else nuke execReplication.c from orbit and
start using the real planner and executor.  Perhaps the foreign
key enforcement mechanisms could be a model --- although if you
don't want to buy into using SPI as well, you probably should look
at Amit L's work at [1].

Also ... maybe I am missing something, but is REPLICA IDENTITY FULL
sanely defined in the first place?  It looks to me that
RelationFindReplTupleSeq assumes without proof that there is a unique
full-tuple match, but that is only reasonable to assume if there is at
least one unique index (and maybe not even then, if nulls are involved).
If there is a unique index, why can't that be chosen as replica identity?
If there isn't, what semantics are we actually providing?

What I'm thinking about is that maybe REPLICA IDENTITY FULL should be
defined as "the subscriber can pick any unique index to match on,
and is allowed to fail if the table has none".  Or if "fail" is a bridge
too far for you, we could fall back to the existing seqscan logic.
But thumbing through the existing indexes to find a non-expression unique
index wouldn't require invoking the full planner.  Any candidate index
would result in a plan estimated to fetch just one row, so there aren't
likely to be serious cost differences.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CA+HiwqG5e8pk8s7+7zhr1Nc_PGyhEdM5f=pHkMOdK1RYWXfJsg@mail.gmail.com

Hi,

On 2023-01-07 13:50:04 -0500, Tom Lane wrote:
> I think we should either live within the constraints set by this
> overarching design, or else nuke execReplication.c from orbit and
> start using the real planner and executor.  Perhaps the foreign
> key enforcement mechanisms could be a model --- although if you
> don't want to buy into using SPI as well, you probably should look
> at Amit L's work at [1].

I don't think using the full executor for every change is feasible from an
overhead perspective. But it might make sense to bail out to using the full
executor in a bunch of non-fastpath paths.

I think this is basically similar to COPY not using the full executor.

But that doesn't mean that all of this has to be open coded in
execReplication.c. Abstracting pieces so that COPY, logical rep and perhaps
even nodeModifyTable.c can share code makes sense.


> Also ... maybe I am missing something, but is REPLICA IDENTITY FULL
> sanely defined in the first place?  It looks to me that
> RelationFindReplTupleSeq assumes without proof that there is a unique
> full-tuple match, but that is only reasonable to assume if there is at
> least one unique index (and maybe not even then, if nulls are involved).

If the table definition match between publisher and standby, it doesn't matter
which tuple is updated, if all columns are used to match. Since there's
nothing distinguishing two rows with all columns being equal, it doesn't
matter which we update.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-01-07 13:50:04 -0500, Tom Lane wrote:
>> Also ... maybe I am missing something, but is REPLICA IDENTITY FULL
>> sanely defined in the first place?  It looks to me that
>> RelationFindReplTupleSeq assumes without proof that there is a unique
>> full-tuple match, but that is only reasonable to assume if there is at
>> least one unique index (and maybe not even then, if nulls are involved).

> If the table definition match between publisher and standby, it doesn't matter
> which tuple is updated, if all columns are used to match. Since there's
> nothing distinguishing two rows with all columns being equal, it doesn't
> matter which we update.

Yeah, but the point here is precisely that they might *not* match;
for example there could be extra columns in the subscriber's table.
This may be largely a documentation problem, though --- I think my
beef is mainly that there's nothing in our docs explaining the
semantic pitfalls of FULL, we only say "it's slow".

Anyway, to get back to the point at hand: if we do have a REPLICA IDENTITY
FULL situation then we can make use of any unique index over a subset of
the transmitted columns, and if there's more than one candidate index
it's unlikely to matter which one we pick.  Given your comment I guess
we have to also compare the non-indexed columns, so we can't completely
convert the FULL case to the straight index case.  But still it doesn't
seem to me to be appropriate to use the planner to find a suitable index.

            regards, tom lane

On Fri, Jan 27, 2023 at 6:32 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>>
>> I suppose the options are:
>> 1. use regular planner uniformly
>> 2. use regular planner only when there's no replica identity (or configurable?)
>> 3. only use low-level functions
>> 4. keep using sequential scans for every single updated row
>> 5. introduce a hidden logical row identifier in the heap that is guaranteed unique within a table and can be used as
areplica identity when no unique index exists 
>
>
> One other option I considered was to ask the index explicitly on the subscriber side from the user when REPLICA
IDENTITYis FULL. But, it is a pretty hard choice for any user, even a planner sometimes fails to pick the right index
:) Also, it is probably controversial to change any of the APIs for this purpose? 
>

I agree that it won't be a very convenient option for the user but how
about along with asking for an index from the user (when the user
didn't provide an index), we also allow to make use of any unique
index over a subset of the transmitted columns, and if there's more
than one candidate index pick any one. Additionally, we can allow
disabling the use of an index scan for this particular case. If we are
too worried about API change for allowing users to specify the index
then we can do that later or as a separate patch.

> I'd be happy to hear from more experienced hackers on the trade-offs for the above, and I'd be open to work on that
ifthere is a clear winner. For me (3) is a decent solution for the problem. 
>

From the discussion above it is not very clear that adding maintenance
costs in this area is worth it even though that can give better
results as far as this feature is concerned.

--
With Regards,
Amit Kapila.

On Thu, Feb 2, 2023 at 2:03 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
>>
>> and if there's more
>> than one candidate index pick any one. Additionally, we can allow
>> disabling the use of an index scan for this particular case. If we are
>> too worried about API change for allowing users to specify the index
>> then we can do that later or as a separate patch.
>>
>
> On v23, I dropped the planner support for picking the index. Instead, it simply
> iterates over the indexes and picks the first one that is suitable.
>
> I'm currently thinking on how to enable users to override this decision.
> One option I'm leaning towards is to add a syntax like the following:
>
> ALTER SUBSCRIPTION .. ALTER TABLE ... SET INDEX ...
>
> Though, that should probably be a seperate patch. I'm going to work
> on that, but still wanted to share v23 given picking the index sounds
> complementary, not strictly required at this point.
>

I agree that it could be a separate patch. However, do you think we
need some way to disable picking the index scan? This is to avoid
cases where sequence scan could be better or do we think there won't
exist such a case?

--
With Regards,
Amit Kapila.

On Thu, Feb 2, 2023 4:34 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
>>
>> and if there's more
>> than one candidate index pick any one. Additionally, we can allow
>> disabling the use of an index scan for this particular case. If we are
>> too worried about API change for allowing users to specify the index
>> then we can do that later or as a separate patch.
>>
>
> On v23, I dropped the planner support for picking the index. Instead, it simply
> iterates over the indexes and picks the first one that is suitable.
>
> I'm currently thinking on how to enable users to override this decision.
> One option I'm leaning towards is to add a syntax like the following:
>
> ALTER SUBSCRIPTION .. ALTER TABLE ... SET INDEX ...
>
> Though, that should probably be a seperate patch. I'm going to work
> on that, but still wanted to share v23 given picking the index sounds
> complementary, not strictly required at this point.
>

Thanks for your patch. Here are some comments.

1.
I noticed that get_usable_indexoid() is called in apply_handle_update_internal()
and apply_handle_delete_internal() to get the usable index. Could usableIndexOid
be a parameter of these two functions? Because we have got the
LogicalRepRelMapEntry when calling them and if we do so, we can get
usableIndexOid without get_usable_indexoid(). Otherwise for partitioned tables,
logicalrep_partition_open() is called in get_usable_indexoid() and searching
the entry via hash_search() will increase cost.

2.
+             * This attribute is an expression, and
+             * SuitableIndexPathsForRepIdentFull() was called earlier when the
+             * index for subscriber was selected. There, the indexes
+             * comprising *only* expressions have already been eliminated.

The comment looks need to be updated:
SuitableIndexPathsForRepIdentFull
->
FindUsableIndexForReplicaIdentityFull

3.

     /* Build scankey for every attribute in the index. */
-    for (attoff = 0; attoff < IndexRelationGetNumberOfKeyAttributes(idxrel); attoff++)
+    for (index_attoff = 0; index_attoff < IndexRelationGetNumberOfKeyAttributes(idxrel);
+         index_attoff++)
     {

Should the comment be changed? Because we skip the attributes that are expressions.

4.
+            Assert(RelationGetReplicaIndex(rel) != RelationGetRelid(idxrel) &&
+                   RelationGetPrimaryKeyIndex(rel) != RelationGetRelid(idxrel));

Maybe we can call the new function idxIsRelationIdentityOrPK()?

Regards,
Shi Yu

On Mon, Feb 13, 2023 7:01 PM shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com> wrote:
> 
> On Thu, Feb 2, 2023 4:34 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> >
> >>
> >> and if there's more
> >> than one candidate index pick any one. Additionally, we can allow
> >> disabling the use of an index scan for this particular case. If we are
> >> too worried about API change for allowing users to specify the index
> >> then we can do that later or as a separate patch.
> >>
> >
> > On v23, I dropped the planner support for picking the index. Instead, it simply
> > iterates over the indexes and picks the first one that is suitable.
> >
> > I'm currently thinking on how to enable users to override this decision.
> > One option I'm leaning towards is to add a syntax like the following:
> >
> > ALTER SUBSCRIPTION .. ALTER TABLE ... SET INDEX ...
> >
> > Though, that should probably be a seperate patch. I'm going to work
> > on that, but still wanted to share v23 given picking the index sounds
> > complementary, not strictly required at this point.
> >
> 
> Thanks for your patch. Here are some comments.
> 

Hi,

Here are some comments on the test cases.

1. in test case "SUBSCRIPTION RE-CALCULATES INDEX AFTER CREATE/DROP INDEX"
+# now, ingest more data and create index on column y which has higher cardinality
+# so that the future commands use the index on column y
+$node_publisher->safe_psql('postgres',
+    "INSERT INTO test_replica_id_full SELECT 50, i FROM generate_series(0,3100)i;");
+$node_subscriber->safe_psql('postgres',
+    "CREATE INDEX test_replica_id_full_idy ON test_replica_id_full(y)");

We don't pick the cheapest index in the current patch, so should we modify this
part of the test?

BTW, the following comment in FindLogicalRepUsableIndex() need to be changed,
too.

+         * We are looking for one more opportunity for using an index. If
+         * there are any indexes defined on the local relation, try to pick
+         * the cheapest index.


2. Is there any reasons why we need the test case "SUBSCRIPTION USES INDEX WITH
DROPPED COLUMNS"? Has there been a problem related to dropped columns before?

3. in test case "SUBSCRIPTION USES INDEX ON PARTITIONED TABLES"
+# deletes rows and moves between partitions
+$node_publisher->safe_psql('postgres',
+    "DELETE FROM users_table_part WHERE user_id = 1 and value_1 = 1;");
+$node_publisher->safe_psql('postgres',
+    "DELETE FROM users_table_part WHERE user_id = 12 and value_1 = 12;");

"moves between partitions" in the comment seems wrong.

4. in test case "SUBSCRIPTION DOES NOT USE INDEXES WITH ONLY EXPRESSIONS"
+# update 2 rows
+$node_publisher->safe_psql('postgres',
+    "UPDATE people SET firstname = 'Nan' WHERE firstname = 'first_name_1';");
+$node_publisher->safe_psql('postgres',
+    "UPDATE people SET firstname = 'Nan' WHERE firstname = 'first_name_2' AND lastname = 'last_name_2';");
+
+# make sure the index is not used on the subscriber
+$result = $node_subscriber->safe_psql('postgres',
+    "select idx_scan from pg_stat_all_indexes where indexrelname = 'people_names'");
+is($result, qq(0), 'ensure subscriber tap_sub_rep_full updates two rows via seq. scan with index on expressions');
+

I think it would be better to call wait_for_catchup() before the check because
we want to check the index is NOT used. Otherwise the check may pass because the
rows have not yet been updated on subscriber.

5. in test case "SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN"
+# show that index is not used even when enable_indexscan=false
+$result = $node_subscriber->safe_psql('postgres',
+    "select idx_scan from pg_stat_all_indexes where indexrelname = 'test_replica_id_full_idx'");
+is($result, qq(0), 'ensure subscriber has not used index with enable_indexscan=false');

Should we remove the word "even" in the comment?

6. 
In each test case we re-create publications, subscriptions, and tables. Could we
create only one publication and one subscription at the beginning, and use them
in all test cases? I think this can save some time running the test file.

Regards,
Shi Yu

Here are some review comments for patch v23.

======
General

1.
IIUC the previous logic for checking "cost" comparisons and selecting
the "cheapest" strategy is no longer present in the latest patch.

In that case, I think there are some leftover stale comments that need
changing. For example,

1a. Commit message:
"let the planner sub-modules compare the costs of index versus
sequential scan and choose the cheapest."

~

1b. Commit message:
"Finally, pick the cheapest `Path` among."

~

1c. FindLogicalRepUsableIndex function:
+ * We are looking for one more opportunity for using an index. If
+ * there are any indexes defined on the local relation, try to pick
+ * the cheapest index.

======
doc/src/sgml/logical-replication.sgml

If replica identity "full" is used, indexes can be used on the
subscriber side for seaching the rows. The index should be btree,
non-partial and have at least one column reference (e.g., should not
consists of only expressions). If there are no suitable indexes, the
search on the subscriber side is very inefficient and should only be
used as a fallback if no other solution is possible

2a.
Fixed typo "seaching", and minor rewording.

SUGGESTION
When replica identity "full" is specified, indexes can be used on the
subscriber side for searching the rows. These indexes should be btree,
non-partial and have at least one column reference (e.g., should not
consist of only expressions). If there are no such suitable indexes,
the search on the subscriber side can be very inefficient, therefore
replica identity "full" should only be used as a fallback if no other
solution is possible.

~

2b.
I know you are just following some existing text here, but IMO this
should probably refer to replica identity <literal>FULL</literal>
instead of "full".

======
src/backend/executor/execReplication.c

3. IdxIsRelationIdentityOrPK

+/*
+ * Given a relation and OID of an index, returns true if
+ * the index is relation's primary key's index or
+ * relaton's replica identity index.
+ *
+ * Returns false otherwise.
+ */
+static bool
+IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid)
+{
+ Assert(OidIsValid(idxoid));
+
+ if (RelationGetReplicaIndex(rel) == idxoid ||
+ RelationGetPrimaryKeyIndex(rel) == idxoid)
+ return true;
+
+ return false;

3a.
Comment typo "relaton"

~

3b.
Code could be written using single like below if you wish (but see #2c)

return RelationGetReplicaIndex(rel) == idxoid ||
RelationGetPrimaryKeyIndex(rel) == idxoid;

~

3c.
Actually, RelationGetReplicaIndex and RelationGetPrimaryKeyIndex
implementations are very similar so it seemed inefficient to be
calling both of them. IMO it might be better to just make a new
relcache function IdxIsRelationIdentityOrPK(Relation rel, Oid idxoid).
This implementation will be similar to those others, but now you need
only to call the workhorse RelationGetIndexList *one* time.

~~~

4. RelationFindReplTupleByIndex

  bool found;
+ TypeCacheEntry **eq = NULL; /* only used when the index is not repl. ident
+ * or pkey */
+ bool idxIsRelationIdentityOrPK;


If you change the comment to say "RI" instead of "repl. Ident" then it
can all fit on one line, which would be an improvement.


======
src/backend/replication/logical/relation.c

5.
 #include "replication/logicalrelation.h"
 #include "replication/worker_internal.h"
+#include "optimizer/cost.h"
 #include "utils/inval.h"

Can that #include be added in alphabetical order like the others or not?

~~~

6. logicalrep_partition_open

+ /*
+ * Finding a usable index is an infrequent task. It occurs when an
+ * operation is first performed on the relation, or after invalidation of
+ * of the relation cache entry (such as ANALYZE or CREATE/DROP index on
+ * the relation).
+ */
+ entry->usableIndexOid = FindLogicalRepUsableIndex(partrel, remoterel);
+

Typo "of of the relation"

~~~

7. FindUsableIndexForReplicaIdentityFull

+static Oid
+FindUsableIndexForReplicaIdentityFull(Relation localrel)
+{
+ MemoryContext usableIndexContext;
+ MemoryContext oldctx;
+ Oid usableIndex;
+ Oid idxoid;
+ List *indexlist;
+ ListCell   *lc;
+ Relation        indexRelation;
+ IndexInfo  *indexInfo;
+ bool is_btree;
+ bool is_partial;
+ bool is_only_on_expression;

It looks like some of these variables are only used within the scope
of the foreach loop, so I think that is where they should be declared.

~~~

8.
+ usableIndex = InvalidOid;

Might as well do that assignment at the declaration.

~~~

9. FindLogicalRepUsableIndex

+ /*
+ * Simple case, we already have a primary key or a replica identity index.
+ *
+ * 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.
+ */
+ idxoid = GetRelationIdentityOrPK(localrel);
+ if (OidIsValid(idxoid))
+ return idxoid;
+
+ /* If index scans are disabled, use a sequential scan */
+ if (!enable_indexscan)
+ return InvalidOid;

~

IMO that "Note" really belongs with the if (!enable)indexscan) more like this:

SUGGESTION
/*
* Simple case, we already have a primary key or a replica identity index.
*/
idxoid = GetRelationIdentityOrPK(localrel);
if (OidIsValid(idxoid))
return idxoid;

/*
* If index scans are disabled, use a sequential scan.
*
* Note we hesitate to move this check to earlier in this function
* because allowing primary key or replica identity even when index scan
* is disabled is the legacy behaviour.
*/
if (!enable_indexscan)
return InvalidOid;

======
src/backend/replication/logical/worker.c

10. get_usable_indexoid

+/*
+ * Decide whether we can pick an index for the relinfo (e.g., the relation)
+ * we're actually deleting/updating from. If it is a child partition of
+ * edata->targetRelInfo, find the index on the partition.
+ *
+ * Note that if the corresponding relmapentry has invalid usableIndexOid,
+ * the function returns InvalidOid.
+ */

"(e.g., the relation)" --> "(i.e. the relation)"

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Sat, Feb 4, 2023 7:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Thu, Feb 2, 2023 at 2:03 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> >
> >>
> >> and if there's more
> >> than one candidate index pick any one. Additionally, we can allow
> >> disabling the use of an index scan for this particular case. If we are
> >> too worried about API change for allowing users to specify the index
> >> then we can do that later or as a separate patch.
> >>
> >
> > On v23, I dropped the planner support for picking the index. Instead, it simply
> > iterates over the indexes and picks the first one that is suitable.
> >
> > I'm currently thinking on how to enable users to override this decision.
> > One option I'm leaning towards is to add a syntax like the following:
> >
> > ALTER SUBSCRIPTION .. ALTER TABLE ... SET INDEX ...
> >
> > Though, that should probably be a seperate patch. I'm going to work
> > on that, but still wanted to share v23 given picking the index sounds
> > complementary, not strictly required at this point.
> >
> 
> I agree that it could be a separate patch. However, do you think we
> need some way to disable picking the index scan? This is to avoid
> cases where sequence scan could be better or do we think there won't
> exist such a case?
> 

I think such a case exists. I tried the following cases based on v23 patch.

# Step 1.
Create publication, subscription and tables.
-- on publisher
create table tbl (a int);
alter table tbl replica identity full;
create publication pub for table tbl;

-- on subscriber
create table tbl (a int);
create index idx_a on tbl(a);
create subscription sub connection 'dbname=postgres port=5432' publication pub;

# Step 2.
Setup synchronous replication.

# Step 3.
Execute SQL query on publisher.

-- case 1 (All values are duplicated)
truncate tbl;
insert into tbl select 1 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 2
truncate tbl;
insert into tbl select i%3 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 3
truncate tbl;
insert into tbl select i%5 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 4
truncate tbl;
insert into tbl select i%10 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 5
truncate tbl;
insert into tbl select i%100 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 6
truncate tbl;
insert into tbl select i%1000 from generate_series(0,10000)i;
update tbl set a=a+1;

-- case 7 (No duplicate value)
truncate tbl;
insert into tbl select i from generate_series(0,10000)i;
update tbl set a=a+1;

# Result
The time executing update (the average of 3 runs is taken, the unit is
milliseconds):

+--------+---------+---------+
|        | patched |  master |
+--------+---------+---------+
| case 1 | 3933.68 | 1298.32 |
| case 2 | 1803.46 | 1294.42 |
| case 3 | 1380.82 | 1299.90 |
| case 4 | 1042.60 | 1300.20 |
| case 5 |  691.69 | 1297.51 |
| case 6 |  578.50 | 1300.69 |
| case 7 |  566.45 | 1302.17 |
+--------+---------+---------+

In case 1~3, there's an overhead after applying the patch. In other cases, the
patch improved the performance. As more duplicate values, the greater the
overhead after applying the patch.

Regards,
Shi Yu

On Wed, Feb 15, 2023 at 9:23 AM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> On Sat, Feb 4, 2023 7:24 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Feb 2, 2023 at 2:03 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> > >
> > > On v23, I dropped the planner support for picking the index. Instead, it simply
> > > iterates over the indexes and picks the first one that is suitable.
> > >
> > > I'm currently thinking on how to enable users to override this decision.
> > > One option I'm leaning towards is to add a syntax like the following:
> > >
> > > ALTER SUBSCRIPTION .. ALTER TABLE ... SET INDEX ...
> > >
> > > Though, that should probably be a seperate patch. I'm going to work
> > > on that, but still wanted to share v23 given picking the index sounds
> > > complementary, not strictly required at this point.
> > >
> >
> > I agree that it could be a separate patch. However, do you think we
> > need some way to disable picking the index scan? This is to avoid
> > cases where sequence scan could be better or do we think there won't
> > exist such a case?
> >
>
> I think such a case exists. I tried the following cases based on v23 patch.
>
...
> # Result
> The time executing update (the average of 3 runs is taken, the unit is
> milliseconds):
>
> +--------+---------+---------+
> |        | patched |  master |
> +--------+---------+---------+
> | case 1 | 3933.68 | 1298.32 |
> | case 2 | 1803.46 | 1294.42 |
> | case 3 | 1380.82 | 1299.90 |
> | case 4 | 1042.60 | 1300.20 |
> | case 5 |  691.69 | 1297.51 |
> | case 6 |  578.50 | 1300.69 |
> | case 7 |  566.45 | 1302.17 |
> +--------+---------+---------+
>
> In case 1~3, there's an overhead after applying the patch. In other cases, the
> patch improved the performance. As more duplicate values, the greater the
> overhead after applying the patch.
>

I think this overhead seems to be mostly due to the need to perform
tuples_equal multiple times for duplicate values. I don't know if
there is any simple way to avoid this without using the planner stuff
as was used in the previous approach. So, this brings us to the
question of whether just providing a way to disable/enable the use of
index scan for such cases is sufficient or if we need any other way.

Tom, Andres, or others, do you have any suggestions on how to move
forward with this patch?

--
With Regards,
Amit Kapila.

FYI, I accidentally left this (v23) patch's TAP test
t/032_subscribe_use_index.pl still lurking even after removing all
other parts of this patch.

In this scenario, the t/032 test gets stuck (build of latest HEAD)

IIUC the patch is only meant to affect performance, so I expected this
032 test to work regardless of whether the rest of the patch is
applied.

Anyway,  it hangs every time for me. I didn't dig looking for the
cause, but if it requires patched code for this new test to pass, I
thought it indicates something wrong either with the test or something
more sinister the new test has exposed. Maybe I am mistaken

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Feb 17, 2023 at 5:57 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> FYI, I accidentally left this (v23) patch's TAP test
> t/032_subscribe_use_index.pl still lurking even after removing all
> other parts of this patch.
>
> In this scenario, the t/032 test gets stuck (build of latest HEAD)
>
> IIUC the patch is only meant to affect performance, so I expected this
> 032 test to work regardless of whether the rest of the patch is
> applied.
>
> Anyway,  it hangs every time for me. I didn't dig looking for the
> cause, but if it requires patched code for this new test to pass, I
> thought it indicates something wrong either with the test or something
> more sinister the new test has exposed. Maybe I am mistaken
>

Sorry, probably the above was a false alarm. After a long time
(minutes) the stuck test did eventually timeout with:

t/032_subscribe_use_index.pl ....... # poll_query_until timed out
executing this query:
# select (idx_scan = 1) from pg_stat_all_indexes where indexrelname =
'test_replica_id_full_idx';
# expecting this output:
# t
# last actual query output:
# f
# with stderr:
t/032_subscribe_use_index.pl ....... Dubious, test returned 29 (wstat
7424, 0x1d00)

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Feb 21, 2023 at 7:55 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
>>
>> I think this overhead seems to be mostly due to the need to perform
>> tuples_equal multiple times for duplicate values. I don't know if
>> there is any simple way to avoid this without using the planner stuff
>> as was used in the previous approach. So, this brings us to the
>> question of whether just providing a way to disable/enable the use of
>> index scan for such cases is sufficient or if we need any other way.
>>
>> Tom, Andres, or others, do you have any suggestions on how to move
>> forward with this patch?
>>
>
> Thanks for the feedback and testing. Due to personal circumstances,
> I could not reply the thread in the last 2 weeks, but I'll be more active
> going forward.
>
>  I also agree that we should have a way to control the behavior.
>
> I created another patch (v24_0001_optionally_disable_index.patch) which can be applied
> on top of v23_0001_use_index_on_subs_when_pub_rep_ident_full.patch.
>
> The new patch adds a new subscription_parameter for both CREATE and ALTER subscription
> named: enable_index_scan. The setting is valid only when REPLICA IDENTITY is full.
>
> What do you think about such a patch to control the behavior? It does not give a per-relation
> level of control, but still useful for many cases.
>

Wouldn't a table-level option like 'apply_index_scan' be better than a
subscription-level option with a default value as false? Anyway, the
bigger point is that we don't see a better way to proceed here than to
introduce some option to control this behavior.

I see this as a way to provide this feature for users but I would
prefer to proceed with this if we can get some more buy-in from senior
community members (at least one more committer) and some user(s) if
possible. So, I once again request others to chime in and share their
opinion.

--
With Regards,
Amit Kapila.

On Mon, Feb 27, 2023 at 12:35 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
>
>> Wouldn't a table-level option like 'apply_index_scan' be better than a
>> subscription-level option with a default value as false? Anyway, the
>> bigger point is that we don't see a better way to proceed here than to
>> introduce some option to control this behavior.
>
>
> What would be a good API for adding such an option for table-level?
> To be more specific, I cannot see any table level sub/pub options in the docs.
>

I was thinking something along the lines of "Storage Parameters" [1]
for a table. See parameters like autovacuum_enabled that decide the
autovacuum behavior for a table. These can be set via CREATE/ALTER
TABLE commands.

[1] - https://www.postgresql.org/docs/devel/sql-createtable.html#SQL-CREATETABLE-STORAGE-PARAMETERS

--
With Regards,
Amit Kapila.

Hi,

On 2023-02-25 16:00:05 +0530, Amit Kapila wrote:
> On Tue, Feb 21, 2023 at 7:55 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> >> I think this overhead seems to be mostly due to the need to perform
> >> tuples_equal multiple times for duplicate values.

I think more work needs to be done to determine the source of the
overhead. It's not clear to me why there'd be an increase in tuples_equal()
calls in the tests upthread.


> Wouldn't a table-level option like 'apply_index_scan' be better than a
> subscription-level option with a default value as false? Anyway, the
> bigger point is that we don't see a better way to proceed here than to
> introduce some option to control this behavior.

I don't think this should default to false. The quadratic apply performance
the sequential scans cause, are a much bigger hazard for users than some apply
performance reqression.


> I see this as a way to provide this feature for users but I would
> prefer to proceed with this if we can get some more buy-in from senior
> community members (at least one more committer) and some user(s) if
> possible. So, I once again request others to chime in and share their
> opinion.

I'd prefer not having an option, because we figure out the cause of the
performance regression (reducing it to be small enough to not care). After
that an option defaulting to using indexes. I don't think an option defaulting
to false makes sense.

I don't care whether it's subscription or relation level option.

Greetings,

Andres Freund

On Wed, Mar 1, 2023 at 12:09 AM Andres Freund <andres@anarazel.de> wrote:
>
> > I see this as a way to provide this feature for users but I would
> > prefer to proceed with this if we can get some more buy-in from senior
> > community members (at least one more committer) and some user(s) if
> > possible. So, I once again request others to chime in and share their
> > opinion.
>
> I'd prefer not having an option, because we figure out the cause of the
> performance regression (reducing it to be small enough to not care). After
> that an option defaulting to using indexes.
>

Sure, if we can reduce regression to be small enough then we don't
need to keep the default as false, otherwise, also, we can consider it
to keep an option defaulting to using indexes depending on the
investigation for regression. Anyway, the main concern was whether it
is okay to have an option for this which I think we have an agreement
on, now I will continue my review.

--
With Regards,
Amit Kapila.

On Wed, Feb 22, 2023 at 7:54 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>

Few comments:
===============
1.
+   identity.  When replica identity <literal>FULL</literal> is specified,
+   indexes can be used on the subscriber side for searching the rows. These
+   indexes should be btree,

Why only btree and not others like a hash index? Also, there should be
some comments in FindUsableIndexForReplicaIdentityFull() to explain
the choices.

2.
- * This is not generic routine, it expects the idxrel to be replication
- * identity of a rel and meet all limitations associated with that.
+ * This is not a generic routine - it expects the idxrel to be an index
+ * that planner would choose if the searchslot includes all the columns
+ * (e.g., REPLICA IDENTITY FULL on the source).
  */
-static bool
+static int

This comment is not clear to me. Which change here makes the
expectation like that? Which planner function/functionality are you
referring to here?

3.
+/*
+ * Given a relation and OID of an index, returns true if
+ * the index is relation's primary key's index or
+ * relation's replica identity index.

It seems the line length is a bit off in the above comments. There
could be a similar mismatch in other places. You might want to run
pgindent.

4.
+}
+
+
+/*
+ * Returns an index oid if there is an index that can be used

Spurious empty line.

5.
- /*
- * We are in error mode so it's fine this is somewhat slow. It's better to
- * give user correct error.
- */
- if (OidIsValid(GetRelationIdentityOrPK(rel->localrel)))
+ /* Give user more precise error if possible. */
+ if (OidIsValid(rel->usableIndexOid))
  {
  ereport(ERROR,
  (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),

Is this change valid? I mean this could lead to the error "publisher
did not send replica identity column expected by the logical
replication target relation" when it should have given an error:
"logical replication target relation \"%s.%s\" has neither REPLICA
IDENTITY index nor PRIMARY ...


--
With Regards,
Amit Kapila.

On Wed, Mar 1, 2023 9:22 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> 
> Hi Andres, Amit, Shi Yu, all
> 
> Andres Freund <mailto:andres@anarazel.de>, 28 Şub 2023 Sal, 21:39 tarihinde şunu yazdı:
> Hi,
> 
> On 2023-02-25 16:00:05 +0530, Amit Kapila wrote:
> > On Tue, Feb 21, 2023 at 7:55 PM Önder Kalacı <mailto:onderkalaci@gmail.com> wrote:
> > >> I think this overhead seems to be mostly due to the need to perform
> > >> tuples_equal multiple times for duplicate values.
> 
> I think more work needs to be done to determine the source of the
> overhead. It's not clear to me why there'd be an increase in tuples_equal()
> calls in the tests upthread.
> 
> You are right, looking closely, in fact, we most of the time do much less 
> tuples_equal() with index scan.
> 
> I've done some profiling with perf, and created flame graphs for the apply worker, with the
> test described above: -- case 1 (All values are duplicated). I used the following commands:
> - perf record -F 99 -p 122555 -g -- sleep 60
> -  perf script | ./http://stackcollapse-perf.pl > out.perf-folded
> -  ./http://flamegraph.pl out.perf-folded > perf_[index|seq]_scan.svg
> 
> I attached both flame graphs. I do not see anything specific regarding what the patch does, but
> instead the difference mostly seems to come down to index scan vs sequential scan related
> functions. As I continue to investigate, I thought it might be useful to share the flame graphs
> so that more experienced hackers could comment on the difference.   
> 
> Regarding my own end-to-end tests: In some runs, the sequential scan is indeed faster for case-1. But, 
> when I execute update tbl set a=a+1; for 50 consecutive times, and measure end to end performance, I see
> much better results for index scan, only case-1 is on-par as mostly I'd expect.
> 
> Case-1, running the update 50 times and waiting all changes applied
> • index scan: 2minutes 36 seconds
> • sequential scan: 2minutes 30 seconds
> Case-2, running the update 50 times and waiting all changes applied
> • index scan: 1 minutes, 2 seconds
> • sequential scan: 2minutes 30 seconds
> Case-7, running the update 50 times and waiting all changes applied
> • index scan: 6 seconds
> • sequential scan: 2minutes 26seconds
> 
> 
> > # Result
> The time executing update (the average of 3 runs is taken, the unit is
> milliseconds):
> 
> Shi Yu, could it be possible for you to re-run the tests with some more runs, and share the average?
> I suspect maybe your test results have a very small pool size, and some runs are making
> the average slightly problematic.
>  
> In my tests, I shared the total time, which is probably also fine.
>

Thanks for your reply, I re-tested (based on
v25_0001_use_index_on_subs_when_pub_rep_ident_full.patch) and took the average
of 100 runs. The results are as follows. The unit is milliseconds.

case1
sequential scan: 1348.57
index scan: 3785.15

case2
sequential scan: 1350.26
index scan: 1754.01

case3
sequential scan: 1350.13
index scan: 1340.97

There was still some degradation in the first two cases. There are some gaps in
our test results. Some information about my test is as follows.

a. Some parameters specified in postgresql.conf.
shared_buffers = 8GB
checkpoint_timeout = 30min
max_wal_size = 20GB
min_wal_size = 10GB
autovacuum = off

b. Executed SQL.
I executed TRUNCATE and INSERT before each UPDATE. I am not sure if you did the
same, or just executed 50 consecutive UPDATEs. If the latter one, there would be
lots of old tuples and this might have a bigger impact on sequential scan. I
tried this case (which executes 50 consecutive UPDATEs) and also saw that the
overhead is smaller than before.


Besides, I looked into the regression of this patch with `gprof`. Some results
are as follows. I think with single buffer lock, sequential scan can scan
multiple tuples (see heapgettup()), while index scan can only scan one tuple. So
in case1, which has lots of duplicate values and more tuples need to be scanned,
index scan takes longer time.

- results of `gprof`
case1:
master
  %   cumulative   self              self     total           
 time   seconds   seconds    calls  ms/call  ms/call  name    
  1.37      0.66     0.01   654312     0.00     0.00  LWLockAttemptLock
  0.00      0.73     0.00   573358     0.00     0.00  LockBuffer
  0.00      0.73     0.00    10014     0.00     0.06  heap_getnextslot

patched
  %   cumulative   self              self     total           
 time   seconds   seconds    calls  ms/call  ms/call  name    
  9.70      1.27     0.36 50531459     0.00     0.00  LWLockAttemptLock
  3.23      2.42     0.12 100259200     0.00     0.00  LockBuffer
  6.20      1.50     0.23 50015101     0.00     0.00  heapam_index_fetch_tuple
  4.04      2.02     0.15 50015101     0.00     0.00  index_fetch_heap
  1.35      3.21     0.05    10119     0.00     0.00  index_getnext_slot

case7:
master
  %   cumulative   self              self     total           
 time   seconds   seconds    calls  ms/call  ms/call  name    
  2.67      0.60     0.02   654582     0.00     0.00  LWLockAttemptLock
  0.00      0.75     0.00   573488     0.00     0.00  LockBuffer
  0.00      0.75     0.00    10014     0.00     0.06  heap_getnextslot

patched
  %   cumulative   self              self     total           
 time   seconds   seconds    calls  ms/call  ms/call  name    
  0.00      0.12     0.00   241979     0.00     0.00  LWLockAttemptLock
  0.00      0.12     0.00   180884     0.00     0.00  LockBuffer
  0.00      0.12     0.00    10101     0.00     0.00  heapam_index_fetch_tuple
  0.00      0.12     0.00    10101     0.00     0.00  index_fetch_heap
  0.00      0.12     0.00    10119     0.00     0.00  index_getnext_slot

Regards,
Shi Yu

On Thu, Mar 2, 2023 at 1:37 PM shiy.fnst@fujitsu.com
<shiy.fnst@fujitsu.com> wrote:
>
> On Wed, Mar 1, 2023 9:22 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
> >
> > > # Result
> > The time executing update (the average of 3 runs is taken, the unit is
> > milliseconds):
> >
> > Shi Yu, could it be possible for you to re-run the tests with some more runs, and share the average?
> > I suspect maybe your test results have a very small pool size, and some runs are making
> > the average slightly problematic.
> >
> > In my tests, I shared the total time, which is probably also fine.
> >
>
> Thanks for your reply, I re-tested (based on
> v25_0001_use_index_on_subs_when_pub_rep_ident_full.patch) and took the average
> of 100 runs. The results are as follows. The unit is milliseconds.
>
> case1
> sequential scan: 1348.57
> index scan: 3785.15
>
> case2
> sequential scan: 1350.26
> index scan: 1754.01
>
> case3
> sequential scan: 1350.13
> index scan: 1340.97
>
> There was still some degradation in the first two cases. There are some gaps in
> our test results. Some information about my test is as follows.
>
> a. Some parameters specified in postgresql.conf.
> shared_buffers = 8GB
> checkpoint_timeout = 30min
> max_wal_size = 20GB
> min_wal_size = 10GB
> autovacuum = off
>
> b. Executed SQL.
> I executed TRUNCATE and INSERT before each UPDATE. I am not sure if you did the
> same, or just executed 50 consecutive UPDATEs. If the latter one, there would be
> lots of old tuples and this might have a bigger impact on sequential scan. I
> tried this case (which executes 50 consecutive UPDATEs) and also saw that the
> overhead is smaller than before.
>
>
> Besides, I looked into the regression of this patch with `gprof`. Some results
> are as follows. I think with single buffer lock, sequential scan can scan
> multiple tuples (see heapgettup()), while index scan can only scan one tuple. So
> in case1, which has lots of duplicate values and more tuples need to be scanned,
> index scan takes longer time.
>
> - results of `gprof`
> case1:
> master
>   %   cumulative   self              self     total
>  time   seconds   seconds    calls  ms/call  ms/call  name
>   1.37      0.66     0.01   654312     0.00     0.00  LWLockAttemptLock
>   0.00      0.73     0.00   573358     0.00     0.00  LockBuffer
>   0.00      0.73     0.00    10014     0.00     0.06  heap_getnextslot
>
> patched
>   %   cumulative   self              self     total
>  time   seconds   seconds    calls  ms/call  ms/call  name
>   9.70      1.27     0.36 50531459     0.00     0.00  LWLockAttemptLock
>   3.23      2.42     0.12 100259200     0.00     0.00  LockBuffer
>   6.20      1.50     0.23 50015101     0.00     0.00  heapam_index_fetch_tuple
>   4.04      2.02     0.15 50015101     0.00     0.00  index_fetch_heap
>   1.35      3.21     0.05    10119     0.00     0.00  index_getnext_slot
>

In the above profile number of calls to index_fetch_heap(),
heapam_index_fetch_tuple() explains the reason for the regression you
are seeing with the index scan. Because the update will generate dead
tuples in the same transaction and those dead tuples won't be removed,
we get those from the index and then need to perform
index_fetch_heap() to find out whether the tuple is dead or not. Now,
for sequence scan also we need to scan those dead tuples but there we
don't need to do back-and-forth between index and heap. I think we can
once check with more number of tuples (say with 20000, 50000, etc.)
for case-1.

--
With Regards,
Amit Kapila.

On Thu, Mar 2, 2023 at 3:00 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>>
>> Few comments:
>> ===============
>> 1.
>> +   identity.  When replica identity <literal>FULL</literal> is specified,
>> +   indexes can be used on the subscriber side for searching the rows. These
>> +   indexes should be btree,
>>
>> Why only btree and not others like a hash index? Also, there should be
>> some comments in FindUsableIndexForReplicaIdentityFull() to explain
>> the choices.
>
>
> I updated the comment(s).
>
> For a more technical reference, we have these restrictions, because we rely on
> RelationFindReplTupleByIndex() which is designed to handle PK/RI. And,
> RelationFindReplTupleByIndex() is written in a way that only expects
> indexes with these limitations.
>
> In order to keep the changes as small as possible, I refrained from relaxing this
> limitation for now. I'm definitely up to working on this for relaxing these
> limitations, and practically allowing more cases for non-unique indexes.
>

See, I think I understand why partial/expression indexes can't be
supported. It seems to me that because the required tuple may not
satisfy the expression and that won't work for our case. But what are
other limitations you see due to which we can't support other index
types for non-unique indexes? Is it just a matter of testing other
index types or there is something more to it, if so, we should add
comments so that they can be supported in the future if it is feasible
to do so.

>
> Attached are both patches: the main patch, and the patch that
> optionally disables the index scans.
>

Both the patches are numbered 0001. It would be better to number them
as 0001 and 0002.



--
With Regards,
Amit Kapila.