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

Hi Amit,

> >
> >>
> >> 4.
> >> +# Testcase start: SUBSCRIPTION BEHAVIOR WITH ENABLE_INDEXSCAN
> >>
> >> As we have removed enable_indexscan check, you should remove this test.
> >
> >
> > Hmm, I think my rebase problems are causing confusion here, which v38
> fixes.
> >
>
> I think it is still not fixed in v38 as the test is still present in 0001.
>

Ah, yes, sorry again for the noise. v39 will drop that.

>
> > In the first commit, we have ENABLE_INDEXSCAN checks. In the second
> commit,
> > I changed the same test to use enable_replica_identity_full_index_scan.
> >
> > If we are going to only consider the first patch to get into the master
> branch,
> > I can probably drop the test. In that case, I'm not sure what is our
> perspective
> > on ENABLE_INDEXSCAN GUC. Do we want to keep that guard in the code
> > (hence the test)?
> >
>
> I am not sure what we are going to do on this because I feel we need
> some storage option as you have in 0002 patch but you and Andres
> thinks that is not required. So, we can discuss that a bit more after
> 0001 is committed but if there is no agreement then we need to
> probably drop it. Even if drop it, I don't think using enable_index
> makes sense. I think for now you don't need to send 0002 patch, let's
> first try to get 0001 patch and then we can discuss about 0002.
>

sounds good, when needed I'll rebase 0002.

> >
> > Also, you have not noted, but I think SUBSCRIPTION USES INDEX WITH
> MULTIPLE COLUMNS
> > already covers SUBSCRIPTION USES INDEX UPDATEs MULTIPLE ROWS.
> >
> > So, I changed the first one to SUBSCRIPTION USES INDEX WITH MULTIPLE
> ROWS AND COLUMNS
> > and dropped the second one. Let me know if it does not make sense to
> you. If I try, there are few more
> > opportunities to squeeze in some more tests together, but those would
> start to complicate readability.
> >
>
> I still want to reduce the test time and will think about it. Which of
> the other tests do you think can be combined?
>
>
I'll follow your suggestion in the next e-mail [2], and focus on further
improvements.

BTW, did you consider updating the patch based on my yesterday's email [1]?
>
>
Yes, replied to that one just now with some wip commits [1]

> It appears you are using inconsistent spacing. It may be better to use
> a single empty line wherever required.
>
>
Sure, let me fix those.

attached v39.

[1]
https://www.postgresql.org/message-id/CACawEhXGnk6v7UOHrxuJjjybHvvq33Zv666ouy4UzjPfJM6tBw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAA4eK1LSYWrthA3xjbrZvZVmwuha10HtM3-QRrVMD7YBt4t3pg%40mail.gmail.com