Re: logical decoding: skip unnecessary snapshot distribution.

On Fri, 16 Oct 2026 at 09:21, yangboyu <yangboyu(dot)yby(at)alibaba-inc(dot)com> wrote:
>
>
> Hi Shlok,
> > For a SHAREDINVALRELCACHE_ID message, dbId == InvalidOid and
> > relId == InvalidOid mean "invalidate the whole relcache". They do not mean
> > that a shared relation changed. CacheInvalidateRelcacheAll() creates this
> > message for publication DDL, such as ALTER PUBLICATION ... SET ALL TABLES.
> >
> > InvalidationsTouchSharedCatalog() returns true for this message. Therefore,
> > for a commit from another database, 'distribute' stays true and the snapshot
> > and invalidations are still distributed.
> >
> > Since this message alone does not mean that a shared catalog changed, should
> > we ignore it when deciding whether to distribute changes from another
> > database?
>
> Your observation is right. And in my idea, it's better to be conservative here.
>
> Considering the meaning of CacheInvalidateRelcacheAll(), all relcaches contain
> the relcaches of shared relations, such as pg_database. So such invalidation
> message should be sent to all backends. While for publication DDLs, as there
> is not a "CacheInvalidateLocalRelcacheAll()", CacheInvalidateRelcacheAll() is a
> reasonable way to invalidate all local tables' relcache.
>
> Although publication ddls are now the only caller of CacheInvalidateRelcacheAll(),
> I think it's more reasonable to regard it as a shared catalog change. And
> LocalExecuteInvalidationMessage() is also in the same way.
>
Hi Boyu,

I agree with you. I reviewed the v2 patch. Here are some comments:

1. I think we can improve the debug message in:
+ elog(DEBUG1,
+ "transaction %u committed in database %u %s catalog changes relevant",
+ xid, parsed->dbId,
+ catalog_changes_relevant ? "has" : "does not have");
Maybe we can change "catalog changes relevant" to relevant catalog changes"?

2. In the 002_two_databases.pl, we can club multiple commands:
+$node->safe_psql('db2', 'CREATE TABLE t2(a int)');
+$node->safe_psql('db2', 'CREATE ROLE regress_role1');
+$node->safe_psql('db2', 'CREATE PUBLICATION pub0 FOR ALL TABLES');
like:
$node->safe_psql(
'db2', qq[
CREATE TABLE t2(a int);
CREATE ROLE regress_role1;
CREATE PUBLICATION pub0 FOR ALL TABLES;
]);
There are multiple places we can do the same.

3. Multiple test use the similar statement:
+my $session1 = $node->background_psql('db1', on_error_stop => 1);
+$session1->query_safe('BEGIN');
+$session1->query_safe('INSERT INTO t1 VALUES (1)');
Maybe we can create a common function for it?

4. The patch requires pgperltidy runs.

I have made changes for the above changes. If you agree feel free to
include the change.

Thanks,
Shlok Kyal