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

On Wed, Jan 13, 2021 at 10:33 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> On Tue, Jan 12, 2021 at 5:23 PM japin <japinli(at)hotmail(dot)com> wrote:
> >
> > On Tue, 12 Jan 2021 at 19:32, Bharath Rupireddy wrote:
> > > On Tue, Jan 12, 2021 at 4:47 PM Li Japin <japinli(at)hotmail(dot)com> wrote:
> > >> IIUC the logical replication only replicate the tables in
publication, I think
> > >> when the tables that aren't in publication should not be replicated.
> > >>
> > >> Attached the patch that fixes it. Thought?
> > >
> > > With that change, we don't get the behaviour that's stated in the
> > > document - "The ADD TABLE and DROP TABLE clauses will add and remove
> > > one or more tables from the publication. Note that adding tables to a
> > > publication that is already subscribed to will require a ALTER
> > > SUBSCRIPTION ... REFRESH PUBLICATION action on the subscribing side in
> > > order to become effective" -
> > > https://www.postgresql.org/docs/devel/sql-alterpublication.html.
> > >
> >
> > The documentation only emphasize adding tables to a publication, not
> > include dropping tables from a publication.
> >
>
> Right and I think that is ensured by the subscriber by calling
> should_apply_changes_for_rel() which won't return true unless the
> newly added relation is not synced via Refresh Publication. So, this
> means with or without this patch we should be sending the changes of
> the newly published table from the publisher?

Oh, my bad, alter subscription...refresh publication is required only when
the tables are added to the publisher. Patch by japin makes the walsender
process to stop sending the data to the subscriber/apply worker. The patch
is based on the idea of looking at the PUBLICATIONRELMAP in
get_rel_sync_entry when the entries have been invalidated in
rel_sync_cache_publication_cb because of alter publication...drop table.
,
When the alter subscription...refresh publication is run on the subscriber,
the SUBSCRIPTIONRELMAP catalogue gets invalidated but the corresponding
cache entries in the LogicalRepRelMap which is used by logicalrep_rel_open
are not invalidated. LogicalRepRelMap is used to know the relations that
are associated with the subscription. But we seem to have not taken care of
invalidating those entries, though we have the invalidation callback
invalidate_syncing_table_states registered for SUBSCRIPTIONRELMAP in
ApplyWorkerMain. So, we miss to work on updating the entries in
LogicalRepRelMap.

IMO, the ideal way to fix this issue is 1) stop the walsender sending the
changes to dropped tables, for this japin patch works 2) we must mark all
the LogicalRepRelMap entries as invalid in invalidate_syncing_table_states
so that in the next logicalrep_rel_open, if the entry is invalidated, then
we have to call GetSubscriptionRelState to get the latest state, as shown
in [1]. Likewise, we might also have to mark the cache entries invalid in
subscription_change_cb which is invalidation callback for pg_subscription

Thoughts?

[1] -
if (entry->state != SUBREL_STATE_READY *|| entry->invalid*)
entry->state = GetSubscriptionRelState(MySubscription->oid,
entry->localreloid,
&entry->statelsn);

if (entry->invalid)
entry->invalid = false;

return entry;

> I have another question on your patch which is why in some cases like
> when we have not inserted in step-5 (as mentioned by you) the
> following insertions are not sent. Is somehow we are setting the
> pubactions as false in that case, if so, how?

The reason is that the issue reported in this thread occurs - when we have
the walsender process running, RelationSyncCache is initialized, we
inserted some data into the table that's sent to the subscriber and the
table is dropped, we miss to set the pubactions to false in
get_rel_sync_entry, though the cache entries have been invalidated.

In some cases, it works properly because the old walsender process was
stopped and when a new walsender is started, then the cache
RelationSyncCache gets initialized again and the pubactions will be set to
false in get_rel_sync_entry.

if (!found)
{
/* immediately make a new entry valid enough to satisfy callbacks */
entry->schema_sent = false;
entry->streamed_txns = NIL;
entry->replicate_valid = false;
entry->pubactions.pubinsert = entry->pubactions.pubupdate =
entry->pubactions.pubdelete = entry->pubactions.pubtruncate =
false;
entry->publish_as_relid = InvalidOid;
}

Hope that clarifies why the issue happens in some cases and not in other
cases.

> > > The publisher stops sending the tuples whenever the relation gets
> > > dropped from the publication, not waiting until alter subscription ...
> > > refresh publication on the subscriber.
> > >
> >
> > If we want to wait the subscriber executing alter subscription ...
refresh publication,
> > maybe we should send some feedback to walsender. How can we send this
feedback to
> > walsender in non-walreceiver process?
> >
>
> I don't think we need this if what I said above is correct.

My bad. This thought was emanated from my poor reading of the documentation.

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