On Fri, Jan 15, 2021 at 11:48 AM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > 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. >
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.
