Subject: RE: Selectively invalidate caches in pgoutput module

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 5 Mar 2025 03:59:19 +0000

On Tuesday, March 4, 2025 7:44 PM Kuroda, Hayato/黒田 隼人 <[email-masked]> 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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 5 Mar 2025 08:43:09 +0000

Dear Hou,

Thanks for reading the thread!

> relsynccache for a single catalog change.

Right. I should have clarified these points on my previous post.

> pg_publication_namespace. Aside from this, 0001 looks good to me.

Thanks for pointing out, fixed. PSA new version.

Additionally, I found a feasibility that discarding whole-cache can be avoided
more. Currently, all caches are unconditionally removed when pg_namesace is updated.
Invalidation is needed to update the qualified name of tables in the replication
messages after the schema renames, but IIUC it is OK to do for relations that
belong to the renamed schema.

IIUC there are two approaches to implement it.
The first idea is to introduce a new type of invalidation message. This is similar
to what 0002 does. The invalidation message can contain relids that belong to
the renaming schema, and pgoutput can register callback for it.
The second idea is to use syscache callback. IIUC, the callback is passed the
hash of oid. Thus, RelSyncCache can contain hash values of its schema, and they
can be compared with a callback function.

I noted the idea on the code as XXX code comment in 0001.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 5 Mar 2025 09:05:25 +0000

Dear hackers,

I did a performance testing with HEAD and v2-0001, and confirmed that it could
improve performance around 20% in the typical case.

Workload
======
I emulated a scenario that there are many tables to be published and only one
table is stop and resume publishing. In HEAD, ALTER PUBLICATION DROP/ADD command
invalidates all entries in the RelSyncCache, but v2 invalidates only a needed
one - this means the decoding time would be reduced after patching.

1. Initialized an instance
2. Created a root table and 5000 leaf tables.
3. Created another table which did not have parent-child relationship.
4. Created a publication which included a root table and another table
5. Created a replication slot with pgoutput plugin.
6. Executed a transaction which would be decoded. In the transaction:
	a. Inserted tuples to all the leaf tables
	b. Altered publication to drop another table
	c. Altered publication again to add the dropped one
	d. Inserted tuples to all the leaf tables again.
7. measured decoding time via SQL interfaces, five times.

Attached script automated above.

Results
=====
Each cell is a median value of 5 runs. Compared with HEAD, V2-0001 can reduce
the decoding time, relatively 20%.

head [ms]	v2-0001 [ms]
252		198

I'm planning to do further tests to prove the benefit for 0002, 0003 patches.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Thu, 6 Mar 2025 15:33:44 +0530

> the decoding time, relatively 20%.

Yeah, this is a good improvement and the patch looks safe to me, so I
pushed with minor changes in the comments.

..
> I'm planning to do further tests to prove the benefit for 0002, 0003 patches.

Ideally, we need the time of only step-6d with and without the patch.
Step 6-a is required to build the cache, but in actual workloads via
walsender, we won't need that, and also, at the end of SQL API
execution, we will forcibly invalidate all caches, so that is also not
required. See, if in the performance measurement of the next set of
patches, you can avoid that.

-- 
With Regards,
Amit Kapila.