Re: Logical Replication - behavior of ALTER PUBLICATION .. DROP TABLE and ALTER SUBSCRIPTION .. REFRESH PUBLICATION

On Fri, Jan 15, 2021 at 11:48 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Thu, Jan 14, 2021 at 10:53 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > 0002 - Invalidates the relation map cache in subscriber syscache
> > > invalidation callbacks. Currently, I'm setting entry->state to
> > > SUBREL_STATE_UNKNOWN in the new invalidation function that's
> > > introduced logicalrep_relmap_invalidate, so that in the next call to
> > > logicalrep_rel_open the entry's state will be read from the system
> > > catalogues using GetSubscriptionRelState. If this is sound's bit
> > > strange, I can add a boolean invalid to LogicalRepRelMapEntry
> > > structure and set that here and in logicalrep_rel_open, I can have
> > > something like:
> > > if (entry->state != SUBREL_STATE_READY || entry->invalid)
> > > entry->state = GetSubscriptionRelState(MySubscription->oid,
> > > entry->localreloid,
> > > &entry->statelsn);
> > >
> > > if (entry->invalid)
> > > entry->invalid = false;
> > >
> >
> > So, the point of the second patch is that it good to have such a
> > thing, otherwise, we don't see any problem if we just use patch-0001,
> > right? I think if we fix the first-one, automatically, we will achieve
> > what we are trying to with the second-one because ultimately
> > logicalrep_relmap_update will be called due to Alter Pub... Drop table
> > .. step and it will mark the entry as SUBREL_STATE_UNKNOWN.
>
> On some further analysis, I found that logicalrep_relmap_update is
> called in subscribers for inserts, delets, updates and truncate
> statements on the dropped(from publication) relations in the
> publisher.
>
> If any alters to pg_subscription, then we invalidate the subscription
> in subscription_change_cb, maybe_reread_subscription subscription will
> take care of re-reading from the system catalogues, see
> apply_handle_XXXX->ensure_transaction -> maybe_reread_subscription.
> And we don't have any entry in LogicalRepRelMapEntry that gets
> affected because of changes to pg_subscription, so we don't need to
> invalidate the rel map cache entries in subscription_change_cb.
>
> If any alters to pg_subscription_rel, then there are two parameters in
> LogicalRepRelMapEntry structure, state and statelsn that may get
> affected. Changes to statelsn column is taken care of with the
> invalidation callback invalidate_syncing_table_states setting
> table_states_valid to true and
> process_syncing_tables->process_syncing_tables_for_apply.
>

I think you are saying the reverse here. The function
invalidate_syncing_table_states sets table_states_valid to false and
the other one sets it to true.

> But, if
> state column is changed somehow (although I'm not quite sure how it
> can change to different states 'i', 'd', 's', 'r' after the initial
> table sync phase in logical replication,
>

These states are for the initial copy-phase after that these won't change.

> but we can pretty much do
> something like update pg_subscription_rel set srsubstate = 'd';), in
> this case invalidate_syncing_table_states gets called, but if we don't
> revalidation of relation map cache entries, they will have the old
> state value.
>

Hmm, modifying state values as you are suggesting have much dire
consequences like in this case it can let tablesync worker to restart
and try to do perform initial-copy again or it can lead to missing
data in subscriber. We don't recommend to change system catalogs
manually due to such problems.

> So what I feel is at least we need the 0002 patch but with only
> invalidating the entries in invalidate_syncing_table_states not in
> subscription_change_cb, although I'm not able to back it up with any
> use case or bug as such.
>

I feel it is better to not do anything for this because neither we
have a test to reproduce the problem nor is it clear from theory if
there is any problem to solve here.

--
With Regards,
Amit Kapila.