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

On Wed, Jan 13, 2021 at 5:15 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Jan 13, 2021 at 3:08 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Wed, Jan 13, 2021 at 2:53 PM Dilip Kumar <dilipbalaut(at)gmail(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?
> > > >
> > >
> > > Instead of doing this, I would expect that the RelationSyncCache entry
> > > should be removed when the relation is dropped from the publication.
> > > So if that is done then it will reload the publication and then it
> > > will not find that that relation as published and it will ignore the
> > > changes. But the patch doesn't seem to be exactly on that line. Am I
> > > missing something here?
> >
> > IIUC, it's not possible to remove the cache entry inside
> > rel_sync_cache_publication_cb, because we don't receive the relid of
> > the alter publication .. dropped relation in the invalidation
> > callback. See the below comment,
> >
> > /*
> > * There is no way to find which entry in our cache the hash belongs to so
> > * mark the whole cache as invalid.
> > */
> > hash_seq_init(&status, RelationSyncCache);
> > while ((entry = (RelationSyncEntry *) hash_seq_search(&status)) != NULL)
> > entry->replicate_valid = false;
>
> So basically your point is that all the RelationSyncEntry are getting
> invalidated right? And if so then in get_rel_sync_entry we will just
> check the updated publication no?
> I am referring to the below code. So ideally this should be already
> working if the relcaches are getting invalidated?
>
> get_rel_sync_entry
> {
> entry = (RelationSyncEntry *) hash_search(RelationSyncCache,
> (void *) &relid,
> HASH_ENTER, &found);
> Assert(entry != NULL);
>
> .....
>
> /* Validate the entry */
> if (!entry->replicate_valid)
> {
> List *pubids = GetRelationPublications(relid);
> ListCell *lc;
> Oid publish_as_relid = relid;
> ... here we will rechek the pulication
> }

Yes, after the entries got invalidated in
rel_sync_cache_publication_cb, we get to the if
(!entry->replicate_valid) part of the code in get_rel_sync_entry, but
in below point, we don't mark the pubactions to false, which were
earlier set to true. Because of this, we will still have pubactions to
true because of which pgoutput_change publishes the changes.

/*
* Don't publish changes for partitioned tables, because
* publishing those of its partitions suffices, unless partition
* changes won't be published due to pubviaroot being set.
*/
if (publish &&
(relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot))
{
entry->pubactions.pubinsert |= pub->pubactions.pubinsert;
entry->pubactions.pubupdate |= pub->pubactions.pubupdate;
entry->pubactions.pubdelete |= pub->pubactions.pubdelete;
entry->pubactions.pubtruncate |= pub->pubactions.pubtruncate;
}

if (entry->pubactions.pubinsert && entry->pubactions.pubupdate &&
entry->pubactions.pubdelete && entry->pubactions.pubtruncate)
break;

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