Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit92c49d1

Browse files
committed
Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRENTLY.
Reconstruction of an SP-GiST index by REINDEX CONCURRENTLY mayinsert some REDIRECT tuples. This will typically happen ina transaction that lacks an XID, which leads either to assertionfailure in spgFormDeadTuple or to insertion of a REDIRECT tuplewith zero xid. The latter's not good either, since eventuallyVACUUM will apply GlobalVisTestIsRemovableXid() to the zero xid,resulting in either an assertion failure or a garbage answer.In practice, since REINDEX CONCURRENTLY locks out index scanstill it's done, it doesn't matter whether it inserts REDIRECTsor PLACEHOLDERs; and likewise it doesn't matter how soon VACUUMreduces such a REDIRECT to a PLACEHOLDER. So in non-assert buildsthere's no observable problem here, other than perhaps a littleindex bloat. But it's not behaving as intended.To fix, remove the failing Assert in spgFormDeadTuple, acknowledgingthat we might sometimes insert a zero XID; and guard VACUUM'sGlobalVisTestIsRemovableXid() call with a test for valid XID,ensuring that we'll reduce such a REDIRECT the first time VACUUMsees it. (Versions before v14 use TransactionIdPrecedes here,which won't fail on zero xid, so they really have no bug at allin non-assert builds.)Another solution could be to not create REDIRECTs at all duringREINDEX CONCURRENTLY, making the relevant code paths treat thatcase like index build (which likewise knows that no concurrentindex scans can be happening). That would allow restoring theAssert in spgFormDeadTuple, but we'd still need the VACUUM changebecause redirection tuples with zero xid may be out there already.But there doesn't seem to be a nice way for spginsert() to tell thatit's being called in REINDEX CONCURRENTLY without some API changes,so we'll leave that as a possible future improvement.In HEAD, also rename the SpGistState.myXid field to redirectXid,which seems less misleading (since it might not in fact be ourtransaction's XID) and is certainly less uninformatively generic.Per bug #18499 from Alexander Lakhin. Back-patch to all supportedbranches.Discussion:https://postgr.es/m/18499-8a519c280f956480@postgresql.org
1 parentba26d15 commit92c49d1

File tree

5 files changed

+33
-11
lines changed

5 files changed

+33
-11
lines changed

‎src/backend/access/spgist/spgutils.c

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -358,8 +358,19 @@ initSpGistState(SpGistState *state, Relation index)
358358
/* Make workspace for constructing dead tuples */
359359
state->deadTupleStorage = palloc0(SGDTSIZE);
360360

361-
/* Set XID to use in redirection tuples */
362-
state->myXid = GetTopTransactionIdIfAny();
361+
/*
362+
* Set horizon XID to use in redirection tuples. Use our own XID if we
363+
* have one, else use InvalidTransactionId. The latter case can happen in
364+
* VACUUM or REINDEX CONCURRENTLY, and in neither case would it be okay to
365+
* force an XID to be assigned. VACUUM won't create any redirection
366+
* tuples anyway, but REINDEX CONCURRENTLY can. Fortunately, REINDEX
367+
* CONCURRENTLY doesn't mark the index valid until the end, so there could
368+
* never be any concurrent scans "in flight" to a redirection tuple it has
369+
* inserted. And it locks out VACUUM until the end, too. So it's okay
370+
* for VACUUM to immediately expire a redirection tuple that contains an
371+
* invalid xid.
372+
*/
373+
state->redirectXid = GetTopTransactionIdIfAny();
363374

364375
/* Assume we're not in an index build (spgbuild will override) */
365376
state->isBuild = false;
@@ -1075,8 +1086,7 @@ spgFormDeadTuple(SpGistState *state, int tupstate,
10751086
if (tupstate == SPGIST_REDIRECT)
10761087
{
10771088
ItemPointerSet(&tuple->pointer, blkno, offnum);
1078-
Assert(TransactionIdIsValid(state->myXid));
1079-
tuple->xid = state->myXid;
1089+
tuple->xid = state->redirectXid;
10801090
}
10811091
else
10821092
{

‎src/backend/access/spgist/spgvacuum.c

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,9 @@ vacuumLeafPage(spgBulkDeleteState *bds, Relation index, Buffer buffer,
188188

189189
/*
190190
* Add target TID to pending list if the redirection could have
191-
* happened since VACUUM started.
191+
* happened since VACUUM started. (If xid is invalid, assume it
192+
* must have happened before VACUUM started, since REINDEX
193+
* CONCURRENTLY locks out VACUUM.)
192194
*
193195
* Note: we could make a tighter test by seeing if the xid is
194196
* "running" according to the active snapshot; but snapmgr.c
@@ -523,8 +525,17 @@ vacuumRedirectAndPlaceholder(Relation index, Relation heaprel, Buffer buffer)
523525

524526
dt = (SpGistDeadTuple) PageGetItem(page, PageGetItemId(page, i));
525527

528+
/*
529+
* We can convert a REDIRECT to a PLACEHOLDER if there could no longer
530+
* be any index scans "in flight" to it. Such an index scan would
531+
* have to be in a transaction whose snapshot sees the REDIRECT's XID
532+
* as still running, so comparing the XID against global xmin is a
533+
* conservatively safe test. If the XID is invalid, it must have been
534+
* inserted by REINDEX CONCURRENTLY, so we can zap it immediately.
535+
*/
526536
if (dt->tupstate == SPGIST_REDIRECT &&
527-
GlobalVisTestIsRemovableXid(vistest, dt->xid))
537+
(!TransactionIdIsValid(dt->xid) ||
538+
GlobalVisTestIsRemovableXid(vistest, dt->xid)))
528539
{
529540
dt->tupstate = SPGIST_PLACEHOLDER;
530541
Assert(opaque->nRedirection > 0);

‎src/backend/access/spgist/spgxlog.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ fillFakeState(SpGistState *state, spgxlogState stateSrc)
3636
{
3737
memset(state, 0, sizeof(*state));
3838

39-
state->myXid = stateSrc.myXid;
39+
state->redirectXid = stateSrc.redirectXid;
4040
state->isBuild = stateSrc.isBuild;
4141
state->deadTupleStorage = palloc0(SGDTSIZE);
4242
}

‎src/include/access/spgist_private.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ typedef struct SpGistState
157157

158158
char *deadTupleStorage;/* workspace for spgFormDeadTuple */
159159

160-
TransactionIdmyXid;/* XID to use when creating a redirect tuple */
160+
TransactionIdredirectXid;/* XID to use when creating a redirect tuple */
161161
boolisBuild;/* true if doing index build */
162162
} SpGistState;
163163

@@ -421,7 +421,8 @@ typedef struct SpGistLeafTupleData
421421
* field, to satisfy some Asserts that we make when replacing a leaf tuple
422422
* with a dead tuple.
423423
* We don't use t_info, but it's needed to align the pointer field.
424-
* pointer and xid are only valid when tupstate = REDIRECT.
424+
* pointer and xid are only valid when tupstate = REDIRECT, and in some
425+
* cases xid can be InvalidTransactionId even then; see initSpGistState.
425426
*/
426427
typedef struct SpGistDeadTupleData
427428
{
@@ -464,7 +465,7 @@ typedef SpGistDeadTupleData *SpGistDeadTuple;
464465

465466
#define STORE_STATE(s, d) \
466467
do { \
467-
(d).myXid = (s)->myXid; \
468+
(d).redirectXid = (s)->redirectXid; \
468469
(d).isBuild = (s)->isBuild; \
469470
} while(0)
470471

‎src/include/access/spgxlog.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
*/
3636
typedef struct spgxlogState
3737
{
38-
TransactionIdmyXid;
38+
TransactionIdredirectXid;
3939
boolisBuild;
4040
} spgxlogState;
4141

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp