On Thu, Jan 14, 2021 at 10:53 AM Amit Kapila <amit.kapil...@gmail.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. 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, 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. 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. Thoughts? With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com