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

Hi,

Thanks for your reply.

jian he <jian.universality@gmail.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.