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

Hi Amit, all

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, 15 Şub 2023 Çar, 07:37 tarihinde
şunu yazdı:

> On Wed, Feb 15, 2023 at 9:23 AM shiy(dot)fnst(at)fujitsu(dot)com
> <shiy(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Sat, Feb 4, 2023 7:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Feb 2, 2023 at 2:03 PM Önder Kalacı <onderkalaci(at)gmail(dot)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?
>
>
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.

(Note that I'll be working on the other feedback in the email thread,
wanted to send this earlier
to hear some early thoughts on v24_0001_optionally_disable_index.patch).