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

Hi Amit, Peter, all

> > If the reason for the stats polling was only to know if some index is
> > chosen or not, I was wondering if you can just convey the same
> > information to the TAP test via some conveniently placed (DEBUG?)
> > logging.
> >
>
> I had thought about it but didn't convince myself that it would be a
> better approach because it would LOG a lot of messages for bulk
> updates/deletes.

I'm also hesitant to add any log messages for testing purposes, especially
something like this one, where a single UPDATE on the source code
leads to an unbounded number of logs.

>
> 1.
> +# subscriber gets the missing table information
> +$node_subscriber->safe_psql('postgres',
> + "ALTER SUBSCRIPTION tap_sub_rep_full REFRESH PUBLICATION");
>
> This and the follow-on test was not required after we have removed
> Dropped columns test.
>
>
Right, I kept it with the idea that we might get the dropped column changes
earlier, so that I can rebase and add the drop column ones.

But, sure, we can add that later to other tests.

> 2. Reduce the number of updates/deletes in the first test to two rows.
>

We don't have any particular reasons to have more tuples. Given the
time constraints, I don't have any objections to change this.

>
> 3. Removed the cases for dropping the index. This ensures that after
> dropping the index on the table we switch to either an index scan (if
> a new index is created) or to a sequence scan. It doesn't seem like a
> very interesting case to me.
>

For that test, my goal was to ensure/show that the invalidation callback
is triggered after `DROP / CREATE INDEX` commands.

Can we always assume that this would never change? Because if this
behavior ever changes, the users would stuck with the wrong/old
index until VACUUM happens.

>
> Apart from the above, I have removed the explicit setting of
> 'wal_retrieve_retry_interval = 1ms' as the same is not done for any
> other subscription tests. I know setting wal_retrieve_retry_interval
> avoids the launcher sometimes taking more time to launch apply worker
> but it is better to be consistent

Hmm, I cannot remember why I added that. It was probably to make
poll_query_until/wait_for_catchup to happen faster.

But, running the test w/wout this setting, I cannot observe any noticeable
difference. So, probably fine to remove.

> . See the changes in
> changes_amit_1.patch, if you agree with the same then please include
> them in the next version.
>

included all, but I'm not very sure to remove (3). If you think we have
coverage for that in other cases, I'm fine with that.

>
> After doing the above, the test time on my machine is closer to what
> other tests take which is ~5s.
>
> Yes, same for me.

Thanks, attaching v46