pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE 6+ messages / 1 participants [nested] [flat]
* pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE @ 2024-06-17 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-06-17 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRENTLY. Reconstruction of an SP-GiST index by REINDEX CONCURRENTLY may insert some REDIRECT tuples. This will typically happen in a transaction that lacks an XID, which leads either to assertion failure in spgFormDeadTuple or to insertion of a REDIRECT tuple with zero xid. The latter's not good either, since eventually VACUUM 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 scans till it's done, it doesn't matter whether it inserts REDIRECTs or PLACEHOLDERs; and likewise it doesn't matter how soon VACUUM reduces such a REDIRECT to a PLACEHOLDER. So in non-assert builds there's no observable problem here, other than perhaps a little index bloat. But it's not behaving as intended. To fix, remove the failing Assert in spgFormDeadTuple, acknowledging that we might sometimes insert a zero XID; and guard VACUUM's GlobalVisTestIsRemovableXid() call with a test for valid XID, ensuring that we'll reduce such a REDIRECT the first time VACUUM sees it. (Versions before v14 use TransactionIdPrecedes here, which won't fail on zero xid, so they really have no bug at all in non-assert builds.) Another solution could be to not create REDIRECTs at all during REINDEX CONCURRENTLY, making the relevant code paths treat that case like index build (which likewise knows that no concurrent index scans can be happening). That would allow restoring the Assert in spgFormDeadTuple, but we'd still need the VACUUM change because redirection tuples with zero xid may be out there already. But there doesn't seem to be a nice way for spginsert() to tell that it'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 our transaction's XID) and is certainly less uninformatively generic. Per bug #18499 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18499-8a519c280f956480@postgresql.org Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/507a900ad32c5df04c59e7b6e36cd4b3e6ce650f Modified Files -------------- src/backend/access/spgist/spgutils.c | 14 ++++++++++++-- src/backend/access/spgist/spgvacuum.c | 12 +++++++++++- src/include/access/spgist_private.h | 3 ++- 3 files changed, 25 insertions(+), 4 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE @ 2024-06-17 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-06-17 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRENTLY. Reconstruction of an SP-GiST index by REINDEX CONCURRENTLY may insert some REDIRECT tuples. This will typically happen in a transaction that lacks an XID, which leads either to assertion failure in spgFormDeadTuple or to insertion of a REDIRECT tuple with zero xid. The latter's not good either, since eventually VACUUM 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 scans till it's done, it doesn't matter whether it inserts REDIRECTs or PLACEHOLDERs; and likewise it doesn't matter how soon VACUUM reduces such a REDIRECT to a PLACEHOLDER. So in non-assert builds there's no observable problem here, other than perhaps a little index bloat. But it's not behaving as intended. To fix, remove the failing Assert in spgFormDeadTuple, acknowledging that we might sometimes insert a zero XID; and guard VACUUM's GlobalVisTestIsRemovableXid() call with a test for valid XID, ensuring that we'll reduce such a REDIRECT the first time VACUUM sees it. (Versions before v14 use TransactionIdPrecedes here, which won't fail on zero xid, so they really have no bug at all in non-assert builds.) Another solution could be to not create REDIRECTs at all during REINDEX CONCURRENTLY, making the relevant code paths treat that case like index build (which likewise knows that no concurrent index scans can be happening). That would allow restoring the Assert in spgFormDeadTuple, but we'd still need the VACUUM change because redirection tuples with zero xid may be out there already. But there doesn't seem to be a nice way for spginsert() to tell that it'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 our transaction's XID) and is certainly less uninformatively generic. Per bug #18499 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18499-8a519c280f956480@postgresql.org Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/92c49d1062f7094f56b80c603233fa4a0ffe2f8b Modified Files -------------- src/backend/access/spgist/spgutils.c | 18 ++++++++++++++---- src/backend/access/spgist/spgvacuum.c | 15 +++++++++++++-- src/backend/access/spgist/spgxlog.c | 2 +- src/include/access/spgist_private.h | 7 ++++--- src/include/access/spgxlog.h | 2 +- 5 files changed, 33 insertions(+), 11 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE @ 2024-06-17 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-06-17 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRENTLY. Reconstruction of an SP-GiST index by REINDEX CONCURRENTLY may insert some REDIRECT tuples. This will typically happen in a transaction that lacks an XID, which leads either to assertion failure in spgFormDeadTuple or to insertion of a REDIRECT tuple with zero xid. The latter's not good either, since eventually VACUUM 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 scans till it's done, it doesn't matter whether it inserts REDIRECTs or PLACEHOLDERs; and likewise it doesn't matter how soon VACUUM reduces such a REDIRECT to a PLACEHOLDER. So in non-assert builds there's no observable problem here, other than perhaps a little index bloat. But it's not behaving as intended. To fix, remove the failing Assert in spgFormDeadTuple, acknowledging that we might sometimes insert a zero XID; and guard VACUUM's GlobalVisTestIsRemovableXid() call with a test for valid XID, ensuring that we'll reduce such a REDIRECT the first time VACUUM sees it. (Versions before v14 use TransactionIdPrecedes here, which won't fail on zero xid, so they really have no bug at all in non-assert builds.) Another solution could be to not create REDIRECTs at all during REINDEX CONCURRENTLY, making the relevant code paths treat that case like index build (which likewise knows that no concurrent index scans can be happening). That would allow restoring the Assert in spgFormDeadTuple, but we'd still need the VACUUM change because redirection tuples with zero xid may be out there already. But there doesn't seem to be a nice way for spginsert() to tell that it'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 our transaction's XID) and is certainly less uninformatively generic. Per bug #18499 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18499-8a519c280f956480@postgresql.org Branch ------ REL_12_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/3e3e2ebea79c6327f1dbf279d1ee894153508558 Modified Files -------------- src/backend/access/spgist/spgutils.c | 14 ++++++++++++-- src/backend/access/spgist/spgvacuum.c | 12 +++++++++++- src/include/access/spgist_private.h | 3 ++- 3 files changed, 25 insertions(+), 4 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE @ 2024-06-17 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-06-17 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRENTLY. Reconstruction of an SP-GiST index by REINDEX CONCURRENTLY may insert some REDIRECT tuples. This will typically happen in a transaction that lacks an XID, which leads either to assertion failure in spgFormDeadTuple or to insertion of a REDIRECT tuple with zero xid. The latter's not good either, since eventually VACUUM 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 scans till it's done, it doesn't matter whether it inserts REDIRECTs or PLACEHOLDERs; and likewise it doesn't matter how soon VACUUM reduces such a REDIRECT to a PLACEHOLDER. So in non-assert builds there's no observable problem here, other than perhaps a little index bloat. But it's not behaving as intended. To fix, remove the failing Assert in spgFormDeadTuple, acknowledging that we might sometimes insert a zero XID; and guard VACUUM's GlobalVisTestIsRemovableXid() call with a test for valid XID, ensuring that we'll reduce such a REDIRECT the first time VACUUM sees it. (Versions before v14 use TransactionIdPrecedes here, which won't fail on zero xid, so they really have no bug at all in non-assert builds.) Another solution could be to not create REDIRECTs at all during REINDEX CONCURRENTLY, making the relevant code paths treat that case like index build (which likewise knows that no concurrent index scans can be happening). That would allow restoring the Assert in spgFormDeadTuple, but we'd still need the VACUUM change because redirection tuples with zero xid may be out there already. But there doesn't seem to be a nice way for spginsert() to tell that it'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 our transaction's XID) and is certainly less uninformatively generic. Per bug #18499 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18499-8a519c280f956480@postgresql.org Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/e4a55378f726c6ad251923687ac1dea3aaaab08a Modified Files -------------- src/backend/access/spgist/spgutils.c | 14 ++++++++++++-- src/backend/access/spgist/spgvacuum.c | 15 +++++++++++++-- src/include/access/spgist_private.h | 3 ++- 3 files changed, 27 insertions(+), 5 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE @ 2024-06-17 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-06-17 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRENTLY. Reconstruction of an SP-GiST index by REINDEX CONCURRENTLY may insert some REDIRECT tuples. This will typically happen in a transaction that lacks an XID, which leads either to assertion failure in spgFormDeadTuple or to insertion of a REDIRECT tuple with zero xid. The latter's not good either, since eventually VACUUM 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 scans till it's done, it doesn't matter whether it inserts REDIRECTs or PLACEHOLDERs; and likewise it doesn't matter how soon VACUUM reduces such a REDIRECT to a PLACEHOLDER. So in non-assert builds there's no observable problem here, other than perhaps a little index bloat. But it's not behaving as intended. To fix, remove the failing Assert in spgFormDeadTuple, acknowledging that we might sometimes insert a zero XID; and guard VACUUM's GlobalVisTestIsRemovableXid() call with a test for valid XID, ensuring that we'll reduce such a REDIRECT the first time VACUUM sees it. (Versions before v14 use TransactionIdPrecedes here, which won't fail on zero xid, so they really have no bug at all in non-assert builds.) Another solution could be to not create REDIRECTs at all during REINDEX CONCURRENTLY, making the relevant code paths treat that case like index build (which likewise knows that no concurrent index scans can be happening). That would allow restoring the Assert in spgFormDeadTuple, but we'd still need the VACUUM change because redirection tuples with zero xid may be out there already. But there doesn't seem to be a nice way for spginsert() to tell that it'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 our transaction's XID) and is certainly less uninformatively generic. Per bug #18499 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18499-8a519c280f956480@postgresql.org Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/f55083319323a752f63306c89c1884f0fcb6a85e Modified Files -------------- src/backend/access/spgist/spgutils.c | 14 ++++++++++++-- src/backend/access/spgist/spgvacuum.c | 15 +++++++++++++-- src/include/access/spgist_private.h | 3 ++- 3 files changed, 27 insertions(+), 5 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE @ 2024-06-17 18:31 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-06-17 18:31 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRENTLY. Reconstruction of an SP-GiST index by REINDEX CONCURRENTLY may insert some REDIRECT tuples. This will typically happen in a transaction that lacks an XID, which leads either to assertion failure in spgFormDeadTuple or to insertion of a REDIRECT tuple with zero xid. The latter's not good either, since eventually VACUUM 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 scans till it's done, it doesn't matter whether it inserts REDIRECTs or PLACEHOLDERs; and likewise it doesn't matter how soon VACUUM reduces such a REDIRECT to a PLACEHOLDER. So in non-assert builds there's no observable problem here, other than perhaps a little index bloat. But it's not behaving as intended. To fix, remove the failing Assert in spgFormDeadTuple, acknowledging that we might sometimes insert a zero XID; and guard VACUUM's GlobalVisTestIsRemovableXid() call with a test for valid XID, ensuring that we'll reduce such a REDIRECT the first time VACUUM sees it. (Versions before v14 use TransactionIdPrecedes here, which won't fail on zero xid, so they really have no bug at all in non-assert builds.) Another solution could be to not create REDIRECTs at all during REINDEX CONCURRENTLY, making the relevant code paths treat that case like index build (which likewise knows that no concurrent index scans can be happening). That would allow restoring the Assert in spgFormDeadTuple, but we'd still need the VACUUM change because redirection tuples with zero xid may be out there already. But there doesn't seem to be a nice way for spginsert() to tell that it'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 our transaction's XID) and is certainly less uninformatively generic. Per bug #18499 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18499-8a519c280f956480@postgresql.org Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/06f81fed3cdd566c4c9767790e9965d17ffbd0c5 Modified Files -------------- src/backend/access/spgist/spgutils.c | 14 ++++++++++++-- src/backend/access/spgist/spgvacuum.c | 15 +++++++++++++-- src/include/access/spgist_private.h | 3 ++- 3 files changed, 27 insertions(+), 5 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-06-17 18:31 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-06-17 18:31 pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE Tom Lane <tgl@sss.pgh.pa.us> 2024-06-17 18:31 pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE Tom Lane <tgl@sss.pgh.pa.us> 2024-06-17 18:31 pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE Tom Lane <tgl@sss.pgh.pa.us> 2024-06-17 18:31 pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE Tom Lane <tgl@sss.pgh.pa.us> 2024-06-17 18:31 pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE Tom Lane <tgl@sss.pgh.pa.us> 2024-06-17 18:31 pgsql: Fix insertion of SP-GiST REDIRECT tuples during REINDEX CONCURRE Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox