RE: Selectively invalidate caches in pgoutput module - Mailing list pgsql-hackers

On Tuesday, March 4, 2025 7:44 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:

Hi,

> 
> > > When the ALTER PUBLICATION command is executed, all entries in
> > RelationSyncCache
> > > will be discarded anyway. This mechanism works well but is sometimes
> > > not
> > efficient.
> > > For example, when the ALTER PUBLICATION DROP TABLE is executed,
> > > 1) the specific entry in RelationSyncCache will be removed, and then
> > > 2) all entries will be discarded twice.
> > >
> >
> > In (2) Why twice? Is it because we call
> > rel_sync_cache_publication_cb() first for PUBLICATIONRELMAP and then
> > for PUBLICATIONOID via publication_invalidation_cb()?
> 
> This part was not correct. I considered that ALTER PUBLICATION DROP TABLE
> also invalidated the pg_publication syscache, but it would not do. So...the
> command firstly invalidate a specific entry, and then invalidate all entries once.
> 
> > > This happens because the pgoutput plugin registers both
> > > RelcacheCallback
> > > (rel_sync_cache_relation_cb) and SyscacheCallback
> > (publication_invalidation_cb,
> > > rel_sync_cache_publication_cb). Then, when ALTER PUBLICATION
> > ADD/SET/DROP is executed,
> > > both the relation cache of added tables and the syscache of
> > > pg_publication_rel
> > and
> > > pg_publication are invalidated.
> > > The callback for the relation cache will remove an entry from the
> > > hash table, and syscache callbacks will look up all entries and
> > > invalidate them. However,
> > AFAICS
> > > does not need to invalidate all of them.
> > >
> >
> > IIUC, you want to say in the above case only (1) would be sufficient, right?
> 
> Right, this is a main aim of this proposal.
> 
> > Is it possible to see the impact? I guess if there are a large number
> > of relations (or partitions), then all will get invalidated even if
> > one relation is added/dropped from the publication; if so, this should
> > impact the performance.
> 
> Agreed. I'm planning to do and share results. Please wait...
> 
> > For Rename/Owner, can we use a new invalidation instead of using
> > relcache invalidation, as that can impact other sessions for no
> > benefit? IIUC, changing the other properties of publication like
> > dropping the table requires us to invalidate even the corresponding
> > relcache entry because it contains the publication descriptor
> > (rd_pubdesc). See RelationBuildPublicationDesc().
> 
> Attached patchset implemented with the approach.
> 0001 contains only part to avoid whole cache invalidation, for ADD/SET/DROP
> command.

I think the changes proposed in 0001 are reasonable. Basically, any
modifications to pg_publication_rel or pg_publication_namespace should trigger
relcache invalidation messages. This is necessary for rebuilding the cache
stored in RelationData::rd_pubdesc, given that these changes could impact
whether a table is published. Following this logic, it should be safe and
appropriate to depend on individual relcache invalidation messages to
invalidate the relsynccache in pgoutput, rather than invalidating the entire
relsynccache for a single catalog change.

Additionally, I've verified the scenario involving partitioned tables to ensure
the invalidation behaves correctly. When a parent table is added to a
publication (with pubviaroot=true), it should influence the publication status
of its child tables as well. And this case works as expected, since
invalidation messages are dispatched for all items in the partition tree
(GetPubPartitionOptionRelations()).

One nitpick for 0001: the comments atop of the rel_sync_cache_publication_cb()
need updating, as they still reference pg_publication_rel and
pg_publication_namespace. Aside from this, 0001 looks good to me.

Best Regards,
Hou zj