Alvaro Herrera wrote:
> Thankfully I found
> another way to solve it, which is to forgo usage of MVCC here and
> instead use SnapshotAny. There's already a mode in
> IndexBuildHeapRangeScan that uses SnapshotAny, but it needs some tweaks
> to do what we need. I'm going to propose a patch along those lines
> shortly.
Here's the promised patch. Includes a new isolation test (which fails
with the old code in two ways: first when using VACUUM it causes the
error message Kevin didn't like to be raised; second when using
brin_summarize_new_values it causes the inserted tuple to not be part of
the summary, which is wrong). This test leaves the pageinspect
extension installed in the isolationtest database, but that seems fine
to me. (No other isolation test installs an extension.)
The error message Kevin complained about is gone, as is some related
commentary. This is replaced by tweaks in IndexBuildHeapRangeScan that
know to do a SnapshotAny scan without being noisy about in progress
insertions/deletions.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 360b26e..de32cca 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -951,9 +951,13 @@ summarize_range(IndexInfo *indexInfo, BrinBuildState *state, Relation heapRel,
* Execute the partial heap scan covering the heap blocks in the specified
* page range, summarizing the heap tuples in it. This scan stops just
* short of brinbuildCallback creating the new index entry.
+ *
+ * Note that it is critical we use the "any visible" mode of
+ * IndexBuildHeapRangeScan here: otherwise, we would miss tuples inserted
+ * by transactions that are still in progress, among other corner cases.
*/
state->bs_currRangeStart = heapBlk;
- IndexBuildHeapRangeScan(heapRel, state->bs_irel, indexInfo, false,
+ IndexBuildHeapRangeScan(heapRel, state->bs_irel, indexInfo, false, true,
heapBlk, state->bs_pagesPerRange,
brinbuildCallback, (void *) state);
@@ -1058,36 +1062,6 @@ brinsummarize(Relation index, Relation heapRel, double *numSummarized,
state = initialize_brin_buildstate(index, revmap,
pagesPerRange);
indexInfo = BuildIndexInfo(index);
-
- /*
- * We only have ShareUpdateExclusiveLock on the table, and
- * therefore other sessions may insert tuples into the range
- * we're going to scan. This is okay, because we take
- * additional precautions to avoid losing the additional
- * tuples; see comments in summarize_range. Set the
- * concurrent flag, which causes IndexBuildHeapRangeScan to
- * use a snapshot other than SnapshotAny, and silences
- * warnings emitted there.
- */
- indexInfo->ii_Concurrent = true;
-
- /*
- * If using transaction-snapshot mode, it would be possible
- * for another transaction to insert a tuple that's not
- * visible to our snapshot if we have already acquired one,
- * when in snapshot-isolation mode; therefore, disallow this
- * from running in such a transaction unless a snapshot hasn't
- * been acquired yet.
- *
- * This code is called by VACUUM and
- * brin_summarize_new_values. Have the error message mention
- * the latter because VACUUM cannot run in a transaction and
- * thus cannot cause this issue.
- */
- if (IsolationUsesXactSnapshot() && FirstSnapshotSet)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
- errmsg("brin_summarize_new_values() cannot run in a transaction that has already obtained a snapshot")));
}
summarize_range(indexInfo, state, heapRel, heapBlk);
@@ -1111,7 +1085,10 @@ brinsummarize(Relation index, Relation heapRel, double *numSummarized,
/* free resources */
brinRevmapTerminate(revmap);
if (state)
+ {
terminate_brin_buildstate(state);
+ pfree(indexInfo);
+ }
}
/*
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 69f35c9..e59b163 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2161,6 +2161,7 @@ IndexBuildHeapScan(Relation heapRelation,
{
return IndexBuildHeapRangeScan(heapRelation, indexRelation,
indexInfo, allow_sync,
+ false,
0, InvalidBlockNumber,
callback, callback_state);
}
@@ -2170,12 +2171,17 @@ IndexBuildHeapScan(Relation heapRelation,
* number of blocks are scanned. Scan to end-of-rel can be signalled by
* passing InvalidBlockNumber as numblocks. Note that restricting the range
* to scan cannot be done when requesting syncscan.
+ *
+ * When "anyvisible" mode is requested, all tuples visible to any transaction
+ * are considered, including those inserted or deleted by transactions that are
+ * still in progress.
*/
double
IndexBuildHeapRangeScan(Relation heapRelation,
Relation indexRelation,
IndexInfo *indexInfo,
bool allow_sync,
+ bool anyvisible,
BlockNumber start_blockno,
BlockNumber numblocks,
IndexBuildCallback callback,
@@ -2210,6 +2216,12 @@ IndexBuildHeapRangeScan(Relation heapRelation,
indexInfo->ii_ExclusionOps != NULL);
/*
+ * "Any visible" mode is not compatible with uniqueness checks; make sure
+ * only one of those is requested.
+ */
+ Assert(!(anyvisible && checking_uniqueness));
+
+ /*
* Need an EState for evaluation of index expressions and partial-index
* predicates. Also a slot to hold the current tuple.
*/
@@ -2236,6 +2248,9 @@ IndexBuildHeapRangeScan(Relation heapRelation,
{
snapshot = RegisterSnapshot(GetTransactionSnapshot());
OldestXmin = InvalidTransactionId; /* not used */
+
+ /* "any visible" mode is not compatible with this */
+ Assert(!anyvisible);
}
else
{
@@ -2364,6 +2379,17 @@ IndexBuildHeapRangeScan(Relation heapRelation,
case HEAPTUPLE_INSERT_IN_PROGRESS:
/*
+ * In "anyvisible" mode, this tuple is visible and we don't
+ * need any further checks.
+ */
+ if (anyvisible)
+ {
+ indexIt = true;
+ tupleIsAlive = true;
+ break;
+ }
+
+ /*
* Since caller should hold ShareLock or better, normally
* the only way to see this is if it was inserted earlier
* in our own transaction. However, it can happen in
@@ -2409,8 +2435,16 @@ IndexBuildHeapRangeScan(Relation heapRelation,
/*
* As with INSERT_IN_PROGRESS case, this is unexpected
- * unless it's our own deletion or a system catalog.
+ * unless it's our own deletion or a system catalog;
+ * but in anyvisible mode, this tuple is visible.
*/
+ if (anyvisible)
+ {
+ indexIt = true;
+ tupleIsAlive = false;
+ break;
+ }
+
xwait = HeapTupleHeaderGetUpdateXid(heapTuple->t_data);
if (!TransactionIdIsCurrentTransactionId(xwait))
{
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 8b3b28d..a29174b 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -105,6 +105,7 @@ extern double IndexBuildHeapRangeScan(Relation heapRelation,
Relation indexRelation,
IndexInfo *indexInfo,
bool allow_sync,
+ bool anyvisible,
BlockNumber start_blockno,
BlockNumber end_blockno,
IndexBuildCallback callback,
diff --git a/src/test/isolation/expected/brin-1.out b/src/test/isolation/expected/brin-1.out
new file mode 100644
index 0000000..ddb90f4
--- /dev/null
+++ b/src/test/isolation/expected/brin-1.out
@@ -0,0 +1,39 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2check s1b s2b s1i s2summ s1c s2c s2check
+step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
+itemoffset blknum attnum allnulls hasnulls placeholder value
+
+1 0 1 f f f {1 .. 1}
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2b: BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT 1;
+?column?
+
+1
+step s1i: INSERT INTO brin_iso VALUES (1000);
+step s2summ: SELECT brin_summarize_new_values('brinidx'::regclass);
+brin_summarize_new_values
+
+1
+step s1c: COMMIT;
+step s2c: COMMIT;
+step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
+itemoffset blknum attnum allnulls hasnulls placeholder value
+
+1 0 1 f f f {1 .. 1}
+2 1 1 f f f {1 .. 1000}
+
+starting permutation: s2check s1b s1i s2vacuum s1c s2check
+step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
+itemoffset blknum attnum allnulls hasnulls placeholder value
+
+1 0 1 f f f {1 .. 1}
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s1i: INSERT INTO brin_iso VALUES (1000);
+step s2vacuum: VACUUM brin_iso;
+step s1c: COMMIT;
+step s2check: SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass);
+itemoffset blknum attnum allnulls hasnulls placeholder value
+
+1 0 1 f f f {1 .. 1}
+2 1 1 f f f {1 .. 1000}
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index c0ed637..5852c6e 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -36,6 +36,7 @@ test: skip-locked
test: skip-locked-2
test: skip-locked-3
test: skip-locked-4
+test: brin-1
test: drop-index-concurrently-1
test: alter-table-1
test: alter-table-2
diff --git a/src/test/isolation/specs/brin-1.spec b/src/test/isolation/specs/brin-1.spec
new file mode 100644
index 0000000..19ac18a
--- /dev/null
+++ b/src/test/isolation/specs/brin-1.spec
@@ -0,0 +1,44 @@
+# This test verifies that values inserted in transactions still in progress
+# are considered during concurrent range summarization (either using the
+# brin_summarize_new_values function or regular VACUUM).
+
+setup
+{
+ CREATE TABLE brin_iso (
+ value int
+ ) WITH (fillfactor=10);
+ CREATE INDEX brinidx ON brin_iso USING brin (value) WITH (pages_per_range=1);
+ -- this fills the first page
+ DO $$
+ DECLARE curtid tid;
+ BEGIN
+ LOOP
+ INSERT INTO brin_iso VALUES (1) RETURNING ctid INTO curtid;
+ EXIT WHEN curtid > tid '(1, 0)';
+ END LOOP;
+ END;
+ $$;
+ CREATE EXTENSION IF NOT EXISTS pageinspect;
+}
+
+teardown
+{
+ DROP TABLE brin_iso;
+}
+
+session "s1"
+step "s1b" { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "s1i" { INSERT INTO brin_iso VALUES (1000); }
+step "s1c" { COMMIT; }
+
+session "s2"
+step "s2b" { BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT 1; }
+step "s2summ" { SELECT brin_summarize_new_values('brinidx'::regclass); }
+step "s2c" { COMMIT; }
+
+step "s2vacuum" { VACUUM brin_iso; }
+
+step "s2check" { SELECT * FROM brin_page_items(get_raw_page('brinidx', 2), 'brinidx'::regclass); }
+
+permutation "s2check" "s1b" "s2b" "s1i" "s2summ" "s1c" "s2c" "s2check"
+permutation "s2check" "s1b" "s1i" "s2vacuum" "s1c" "s2check"
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
