Re: [PATCH]: Not to invaldiate CatalogSnapshot for local invalidation messages

Hi,
Thanks for your reply.

jian he <jian(dot)universality(at)gmail(dot)com> 于2023年12月18日周一 08:20写道：

> Hi
> ---setup.
> drop table s2;
> create table s2(a int);
>
> After apply the patch
> alter table s2 add primary key (a);
>
> watch CatalogSnapshot
> ----
> #0 GetNonHistoricCatalogSnapshot (relid=1259)
> at
> ../../Desktop/pg_src/src7/postgresql/src/backend/utils/time/snapmgr.c:412
> #1 0x000055ba78f0d6ba in GetCatalogSnapshot (relid=1259)
> at
> ../../Desktop/pg_src/src7/postgresql/src/backend/utils/time/snapmgr.c:371
> #2 0x000055ba785ffbe1 in systable_beginscan
> (heapRelation=0x7f256f30b5a8, indexId=2662, indexOK=false,
> snapshot=0x0, nkeys=1, key=0x7ffe230f0180)
> at
> ../../Desktop/pg_src/src7/postgresql/src/backend/access/index/genam.c:413
> (More stack frames follow...)
>
> -------------------------
> Hardware watchpoint 13: CatalogSnapshot
>
> Old value = (Snapshot) 0x55ba7980b6a0 <CatalogSnapshotData>
> New value = (Snapshot) 0x0
> InvalidateCatalogSnapshot () at
> ../../Desktop/pg_src/src7/postgresql/src/backend/utils/time/snapmgr.c:435
> 435 SnapshotResetXmin();
> (gdb) bt 4
> #0 InvalidateCatalogSnapshot ()
> at
> ../../Desktop/pg_src/src7/postgresql/src/backend/utils/time/snapmgr.c:435
> #1 0x000055ba78f0ee85 in AtEOXact_Snapshot (isCommit=true,
> resetXmin=false)
> at
> ../../Desktop/pg_src/src7/postgresql/src/backend/utils/time/snapmgr.c:1057
> #2 0x000055ba7868201b in CommitTransaction ()
> at
> ../../Desktop/pg_src/src7/postgresql/src/backend/access/transam/xact.c:2373
> #3 0x000055ba78683495 in CommitTransactionCommand ()
> at
> ../../Desktop/pg_src/src7/postgresql/src/backend/access/transam/xact.c:3061
> (More stack frames follow...)
>
> --
> but the whole process changes pg_class, pg_index,
> pg_attribute,pg_constraint etc.
> only one GetCatalogSnapshot and InvalidateCatalogSnapshot seems not
> correct?
> what if there are concurrency changes in the related pg_catalog table.
>
> your patch did pass the isolation test!
>

Yes, I have run the installcheck-world locally, and all the tests passed.
There are two kinds of Invalidation Messages.
One kind is from the local backend, such as what you did in the example
"alter table s2 add primary key (a);", it modifies the pg_class,
pg_attribute ect ,
so it generates some Invalidation Messages to invalidate the "s2" related
tuples in pg_class , pg_attribute ect, and Invalidate Message to invalidate
s2
relation cache. When the command is finished, in the
CommandCounterIncrement,
those Invalidation Messages will be processed to make the system cache work
well for the following commands.

The other kind of Invalidation Messages are from other backends.
Suppose there are two sessions:
session1
---
1: create table foo(a int);
---
session 2
---
1: create table test(a int); (before session1:1)
2: insert into foo values(1); (execute after session1:1)
---
Session1 will generate Invalidation Messages and send them when the
transaction is committed,
and session 2 will accept those Invalidation Messages from session 1 and
then execute
the second command.

Before the patch, Postgres will invalidate the CatalogSnapshot for those
two kinds of Invalidation
Messages. So I did a small optimization in this patch, for local
Invalidation Messages, we don't
call InvalidateCatalogSnapshot, we can use one CatalogSnapshot in a
transaction even if we modify
the catalog and generate Invalidation Messages, as the visibility of the
tuple is identified by the curcid,
as long as we update the curcid of the CatalogSnapshot in
SnapshotSetCommandId,
it can work
correctly.

> I think you patch doing is against following code comments in
> src/backend/utils/time/snapmgr.c
>
> /*
> * CurrentSnapshot points to the only snapshot taken in
> transaction-snapshot
> * mode, and to the latest one taken in a read-committed transaction.
> * SecondarySnapshot is a snapshot that's always up-to-date as of the
> current
> * instant, even in transaction-snapshot mode. It should only be used for
> * special-purpose code (say, RI checking.) CatalogSnapshot points to an
> * MVCC snapshot intended to be used for catalog scans; we must invalidate
> it
> * whenever a system catalog change occurs.
> *
> * These SnapshotData structs are static to simplify memory allocation
> * (see the hack in GetSnapshotData to avoid repeated malloc/free).
> */
> static SnapshotData CurrentSnapshotData = {SNAPSHOT_MVCC};
> static SnapshotData SecondarySnapshotData = {SNAPSHOT_MVCC};
> SnapshotData CatalogSnapshotData = {SNAPSHOT_MVCC};
> SnapshotData SnapshotSelfData = {SNAPSHOT_SELF};
> SnapshotData SnapshotAnyData = {SNAPSHOT_ANY};
>

Thank you for pointing it out, I think I need to update the comment in the
patch too.