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

On Wed, Jan 13, 2021 at 3:16 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Jan 13, 2021 at 2:36 PM japin <japinli(at)hotmail(dot)com> wrote:
> > > In summary, I feel we need to fix the publisher sending the inserts
> > > even though the table is dropped from the publication, that is the
> > > patch patch proposed by japin. This solves the bug reported in this
> > > thread.
> > >
> > > And also, it's good to have the LogicalRepRelMap invalidation fix as a
> > > 0002 patch in invalidate_syncing_table_states, subscription_change_cb
> > > and logicalrep_rel_open as proposed by me.
> > >
> > > Thoughts?
> > >
> >
> > I think invalidate the LogicalRepRelMap is necessary. If the table isn't in
> > subscription, can we remove the LogicalRepRelMapEntry from LogicalRepRelMap?
>
> IIUC, it's not possible to know the relid of the alter
> publication...dropped table in the invalidation callbacks
> invalidate_syncing_table_states or subscription_change_cb, so it's
> better to set state of all the entries to SUBREL_STATE_UNKNOWN, so
> that, in the next logicalrep_rel_open call the function
> GetSubscriptionRelState will be called and the pg_subscription_rel
> will be read freshly.
>
> This is inline with the reasoning specified at [1].
>
> [1] - https://www.postgresql.org/message-id/CAFiTN-udwuc_h0TaFrSEKb-Yo1vVvkjz9TDRw7VE3P-KiPSGbQ%40mail.gmail.com

Attaching following two patches:

0001 - Makes publisher to not publish the changes for the alter
publication ... dropped tables. Original patch is by japin, I added
comments, changed the function name and adjusted the code a bit.
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;

make check and make check-world passes on both patches.

If okay with the fixes, I will try to add a test case and also a cf
entry so that these patches get a chance to be tested on all the
platforms.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com