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

Hi,
I updated the comment about the CatalogSnapshot `src/backend/utils/time/
snapmgr.c`

Xiaoran Wang <fanfuxiaoran(at)gmail(dot)com> 于2023年12月18日周一 15:02写道：

> 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.
>