On Fri, 16 Oct 2026 at 09:21, yangboyu <[email protected]> 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
comments.patch
Description: Binary data
