From c7afd67c8d48f769f2d5c7b945fc76f9a61d4efe Mon Sep 17 00:00:00 2001 From: liushengsong Date: Thu, 3 Aug 2023 21:42:19 +0800 Subject: [PATCH 1/2] FIX: constraint check exception with snapshot memory leak in AO/AOCS. _bt_check_unique(DirtySnapshot is created here) -> table_index_fetch_tuple_check -> ... -> AppendOnlyVisimapStore_Init -> RegisterSnapshot(snapshot) UnregisterSnapshot is called in AppendOnlyVisimapStore_Finish. If transaction aborts, there is no chance to call AppendOnlyVisimapStore_Finish. We passed in an InvalidSnapshot in Visimap initialization. And in order to ensure that Visimap can get the correct snapshot, we assign the snapshot in the execution time. --- src/backend/access/aocs/aocsam.c | 4 +-- src/backend/access/aocs/aocsam_handler.c | 9 ++++-- src/backend/access/appendonly/appendonlyam.c | 2 +- .../access/appendonly/appendonlyam_handler.c | 4 +++ src/test/regress/input/constraints.source | 32 +++++++++++++++++++ src/test/regress/output/constraints.source | 30 +++++++++++++++++ 6 files changed, 76 insertions(+), 5 deletions(-) diff --git a/src/backend/access/aocs/aocsam.c b/src/backend/access/aocs/aocsam.c index 267dd837697..090d1cc92b8 100644 --- a/src/backend/access/aocs/aocsam.c +++ b/src/backend/access/aocs/aocsam.c @@ -1564,8 +1564,8 @@ aocs_fetch_init(Relation relation, visimaprelid, visimapidxid, AccessShareLock, - appendOnlyMetaDataSnapshot); - + InvalidSnapshot); + return aocsFetchDesc; } diff --git a/src/backend/access/aocs/aocsam_handler.c b/src/backend/access/aocs/aocsam_handler.c index 5e20b685d20..fcae2f877a2 100644 --- a/src/backend/access/aocs/aocsam_handler.c +++ b/src/backend/access/aocs/aocsam_handler.c @@ -705,6 +705,7 @@ aoco_index_fetch_tuple(struct IndexFetchTableData *scan, bool *call_again, bool *all_dead) { IndexFetchAOCOData *aocoscan = (IndexFetchAOCOData *) scan; + bool found = false; if (!aocoscan->aocofetch) { @@ -740,15 +741,19 @@ aoco_index_fetch_tuple(struct IndexFetchTableData *scan, */ Assert(aocoscan->aocofetch->snapshot == snapshot); + aocoscan->aocofetch->visibilityMap.visimapStore.snapshot = snapshot; + ExecClearTuple(slot); if (aocs_fetch(aocoscan->aocofetch, (AOTupleId *) tid, slot)) { ExecStoreVirtualTuple(slot); - return true; + found = true; } - return false; + aocoscan->aocofetch->visibilityMap.visimapStore.snapshot = InvalidSnapshot; + + return found; } static void diff --git a/src/backend/access/appendonly/appendonlyam.c b/src/backend/access/appendonly/appendonlyam.c index 4d84e957026..d4441950d13 100755 --- a/src/backend/access/appendonly/appendonlyam.c +++ b/src/backend/access/appendonly/appendonlyam.c @@ -2281,7 +2281,7 @@ appendonly_fetch_init(Relation relation, aoFormData.visimaprelid, aoFormData.visimapidxid, AccessShareLock, - appendOnlyMetaDataSnapshot); + InvalidSnapshot); return aoFetchDesc; diff --git a/src/backend/access/appendonly/appendonlyam_handler.c b/src/backend/access/appendonly/appendonlyam_handler.c index 12dce83d488..0c1c4858acc 100644 --- a/src/backend/access/appendonly/appendonlyam_handler.c +++ b/src/backend/access/appendonly/appendonlyam_handler.c @@ -545,8 +545,12 @@ appendonly_index_fetch_tuple(struct IndexFetchTableData *scan, */ Assert(aoscan->aofetch->snapshot == snapshot); + aoscan->aofetch->visibilityMap.visimapStore.snapshot = snapshot; + appendonly_fetch(aoscan->aofetch, (AOTupleId *) tid, slot); + aoscan->aofetch->visibilityMap.visimapStore.snapshot = InvalidSnapshot; + return !TupIsNull(slot); } diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source index e3afd458c27..3346edc1ae1 100644 --- a/src/test/regress/input/constraints.source +++ b/src/test/regress/input/constraints.source @@ -591,3 +591,35 @@ DROP DOMAIN constraint_comments_dom; DROP ROLE regress_constraint_comments; DROP ROLE regress_constraint_comments_noaccess; + +-- +-- EXCLUDE constraints for AO +-- +CREATE TABLE test_exclude_ao( + id int4, + room int4range, + EXCLUDE USING gist (room WITH =) +) USING AO_ROW DISTRIBUTED REPLICATED; +INSERT INTO test_exclude_ao VALUES(1, int4range(123, 123, '[]')); + +-- conflicting key value violates exclusion constraint +-- expected conflicting key error without other exception happen +INSERT INTO test_exclude_ao VALUES(1, int4range(123, 123, '[]')); + +DROP TABLE test_exclude_ao; + +-- +-- EXCLUDE constraints for AOCS +-- +CREATE TABLE test_exclude_aocs( + id int4, + room int4range, + EXCLUDE USING gist (room WITH =) +) USING AO_COLUMN DISTRIBUTED REPLICATED; +INSERT INTO test_exclude_aocs VALUES(1, int4range(123, 123, '[]')); + +-- conflicting key value violates exclusion constraint +-- expected conflicting key error without other exception happen +INSERT INTO test_exclude_aocs VALUES(1, int4range(123, 123, '[]')); + +DROP TABLE test_exclude_aocs; diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source index 3ab2def4026..8e9a46ab1ee 100644 --- a/src/test/regress/output/constraints.source +++ b/src/test/regress/output/constraints.source @@ -779,3 +779,33 @@ DROP TABLE constraint_comments_tbl; DROP DOMAIN constraint_comments_dom; DROP ROLE regress_constraint_comments; DROP ROLE regress_constraint_comments_noaccess; +-- +-- EXCLUDE constraints for AO +-- +CREATE TABLE test_exclude_ao( + id int4, + room int4range, + EXCLUDE USING gist (room WITH =) +) USING AO_ROW DISTRIBUTED REPLICATED; +INSERT INTO test_exclude_ao VALUES(1, int4range(123, 123, '[]')); +-- conflicting key value violates exclusion constraint +-- expected conflicting key error without other exception happen +INSERT INTO test_exclude_ao VALUES(1, int4range(123, 123, '[]')); +DETAIL: Key (room)=([123,124)) conflicts with existing key (room)=([123,124)). +ERROR: conflicting key value violates exclusion constraint "test_exclude_ao_room_excl" +DROP TABLE test_exclude_ao; +-- +-- EXCLUDE constraints for AOCS +-- +CREATE TABLE test_exclude_aocs( + id int4, + room int4range, + EXCLUDE USING gist (room WITH =) +) USING AO_COLUMN DISTRIBUTED REPLICATED; +INSERT INTO test_exclude_aocs VALUES(1, int4range(123, 123, '[]')); +-- conflicting key value violates exclusion constraint +-- expected conflicting key error without other exception happen +INSERT INTO test_exclude_aocs VALUES(1, int4range(123, 123, '[]')); +DETAIL: Key (room)=([123,124)) conflicts with existing key (room)=([123,124)). +ERROR: conflicting key value violates exclusion constraint "test_exclude_aocs_room_excl" +DROP TABLE test_exclude_aocs; From da2b3667f324b8ad46acc0eda07e6db678d4e8d4 Mon Sep 17 00:00:00 2001 From: liushengsong Date: Tue, 8 Aug 2023 11:28:12 +0800 Subject: [PATCH 2/2] FIX: snapshot assign in aoco_scan_bitmap_next_tuple --- src/backend/access/aocs/aocsam_handler.c | 5 ++++- src/backend/access/appendonly/appendonlyam_handler.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/backend/access/aocs/aocsam_handler.c b/src/backend/access/aocs/aocsam_handler.c index fcae2f877a2..2052c477c2f 100644 --- a/src/backend/access/aocs/aocsam_handler.c +++ b/src/backend/access/aocs/aocsam_handler.c @@ -1813,6 +1813,8 @@ aoco_scan_bitmap_next_tuple(TableScanDesc scan, aocsBitmapScan->bitmapScanDesc[aocsBitmapScan->whichDesc].bitmapFetch = aocoFetchDesc; } + aocoFetchDesc->visibilityMap.visimapStore.snapshot = aocsBitmapScan->appendOnlyMetaDataSnapshot; + ExecClearTuple(slot); /* ntuples == -1 indicates a lossy page */ @@ -1847,11 +1849,12 @@ aoco_scan_bitmap_next_tuple(TableScanDesc scan, /* OK to return this tuple */ ExecStoreVirtualTuple(slot); pgstat_count_heap_fetch(aocsBitmapScan->rs_base.rs_rd); + aocoFetchDesc->visibilityMap.visimapStore.snapshot = InvalidSnapshot; return true; } } - + aocoFetchDesc->visibilityMap.visimapStore.snapshot = InvalidSnapshot; /* Done with this block */ return false; } diff --git a/src/backend/access/appendonly/appendonlyam_handler.c b/src/backend/access/appendonly/appendonlyam_handler.c index 0c1c4858acc..4593c615b6a 100644 --- a/src/backend/access/appendonly/appendonlyam_handler.c +++ b/src/backend/access/appendonly/appendonlyam_handler.c @@ -1978,6 +1978,8 @@ appendonly_scan_bitmap_next_tuple(TableScanDesc scan, } + aoscan->aofetch->visibilityMap.visimapStore.snapshot = aoscan->appendOnlyMetaDataSnapshot; + ExecClearTuple(slot); /* ntuples == -1 indicates a lossy page */ @@ -2011,11 +2013,12 @@ appendonly_scan_bitmap_next_tuple(TableScanDesc scan, { /* OK to return this tuple */ pgstat_count_heap_fetch(aoscan->aos_rd); + aoscan->aofetch->visibilityMap.visimapStore.snapshot = InvalidSnapshot; return true; } } - + aoscan->aofetch->visibilityMap.visimapStore.snapshot = InvalidSnapshot; /* Done with this block */ return false; }