Re: Logical Replication - behavior of ALTER PUBLICATION .. DROP TABLE and ALTER SUBSCRIPTION .. REFRESH PUBLICATION - Mailing list pgsql-hackers

On Wed, Jan 13, 2021 at 5:40 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Jan 13, 2021 at 3:16 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, Jan 13, 2021 at 2:36 PM japin <japinli@hotmail.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.
>

Do we really need to access PUBLICATIONRELMAP in this patch? What if
we just set it to false in the else condition of (if (publish &&
(relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot)))

> 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.

> 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.
>

I think it is good to follow both the steps (adding a testcase and
registering it to CF).

-- 
With Regards,
Amit Kapila.