public inbox for [email protected]help / color / mirror / Atom feed
Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. 6+ messages / 3 participants [nested] [flat]
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. @ 2023-03-27 17:17 Robert Haas <[email protected]> 2023-03-27 18:34 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Peter Geoghegan <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Robert Haas @ 2023-03-27 17:17 UTC (permalink / raw) To: Peter Geoghegan <[email protected]>; +Cc: pgsql-hackers On Sat, Mar 25, 2023 at 6:25 PM Peter Geoghegan <[email protected]> wrote: > On Fri, Mar 24, 2023 at 8:13 AM Robert Haas <[email protected]> wrote: > > If we're checking xmin and find that it is invalid (i.e. 0) just > > report that as corruption, similar to what's already done in the > > three cases that seem correct. If we're checking xmax and find > > that's invalid, that's fine: it just means that the tuple hasn't > > been updated or deleted. > > What about aborted speculative insertions? See > heap_abort_speculative(), which directly sets the speculatively > inserted heap tuple's xmin to InvalidTransactionId/zero. Oh, dear. I didn't know about that case. > It probably does make sense to keep something close to this check -- > it just needs to account for speculative insertions to avoid false > positive reports of corruption. We could perform cross-checks against > a tuple whose xmin is InvalidTransactionId/zero to verify that it > really is from an aborted speculative insertion, to the extent that > that's possible. For example, such a tuple can't be a heap-only tuple, > and it can't have any xmax value other than InvalidTransactionId/zero. Since this was back-patched, I think it's probably better to just remove the error. We can introduce new validation if we want, but that should probably be master-only. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. 2023-03-27 17:17 Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> @ 2023-03-27 18:34 ` Peter Geoghegan <[email protected]> 2023-03-27 20:17 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Peter Geoghegan @ 2023-03-27 18:34 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Mon, Mar 27, 2023 at 10:17 AM Robert Haas <[email protected]> wrote: > > What about aborted speculative insertions? See > > heap_abort_speculative(), which directly sets the speculatively > > inserted heap tuple's xmin to InvalidTransactionId/zero. > > Oh, dear. I didn't know about that case. A big benefit of having extensive amcheck coverage is that it effectively centralizes information about the on-disk format, in an easy to understand way, and (over time) puts things on a more rigorous footing. Now it'll be a lot harder for somebody else to overlook that case in the future, which is good. Things are trending in the right direction. > > It probably does make sense to keep something close to this check -- > > it just needs to account for speculative insertions to avoid false > > positive reports of corruption. We could perform cross-checks against > > a tuple whose xmin is InvalidTransactionId/zero to verify that it > > really is from an aborted speculative insertion, to the extent that > > that's possible. For example, such a tuple can't be a heap-only tuple, > > and it can't have any xmax value other than InvalidTransactionId/zero. > > Since this was back-patched, I think it's probably better to just > remove the error. We can introduce new validation if we want, but that > should probably be master-only. That makes sense. I don't think that it's particularly likely that having refined aborted speculative insertion amcheck coverage will make a critical difference to any user, at any time. But "amcheck as documentation of the on-disk format" is reason enough to have it. -- Peter Geoghegan ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. 2023-03-27 17:17 Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> 2023-03-27 18:34 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Peter Geoghegan <[email protected]> @ 2023-03-27 20:17 ` Robert Haas <[email protected]> 2023-03-27 20:51 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Peter Geoghegan <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Robert Haas @ 2023-03-27 20:17 UTC (permalink / raw) To: Peter Geoghegan <[email protected]>; +Cc: pgsql-hackers On Mon, Mar 27, 2023 at 2:34 PM Peter Geoghegan <[email protected]> wrote: > > Since this was back-patched, I think it's probably better to just > > remove the error. We can introduce new validation if we want, but that > > should probably be master-only. > > That makes sense. Patch attached. > I don't think that it's particularly likely that having refined > aborted speculative insertion amcheck coverage will make a critical > difference to any user, at any time. But "amcheck as documentation of > the on-disk format" is reason enough to have it. Sure, if someone feels like writing the code. I have to admit that I have mixed feelings about this whole direction. In concept, I agree with you entirely: a fringe benefit of having checks that tell us whether or not a page is valid is that it helps to make clear what page states we think are valid. In practice, however, the point you raise in your first sentence weighs awfully heavily with me. Spending a lot of energy on checks that are unlikely to catch practical problems feels like it may not be the best use of time. I'm not sure exactly where to draw the line, but it seems highly likely to be that there are things we could deduce about the page that wouldn't be worth the effort. For example, would we bother checking that a tuple with an in-progress xmin does not have a smaller natts value than a tuple with a committed xmin? Or that natts values are non-decreasing across a HOT chain? I suspect there are even more obscure examples of things that should be true but might not really be worth worrying about in the code. -- Robert Haas EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] 0001-amcheck-In-verify_heapam-allows-tuples-with-xmin-0.patch (1.2K, ../../CA+Tgmobi9X1UX8sfLQ4SS5u4TfJgg3-v5CX2HDs2Z73oHGr_Aw@mail.gmail.com/2-0001-amcheck-In-verify_heapam-allows-tuples-with-xmin-0.patch) download | inline diff: From 199b1468a67bdbe6e8493339fc78f5bd1d406241 Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Mon, 27 Mar 2023 14:36:03 -0400 Subject: [PATCH] amcheck: In verify_heapam, allows tuples with xmin 0. Commit e88754a1965c0f40a723e6e46d670cacda9e19bd caused that case to be reported as corruption, but Peter Geoghegan pointed out that it can legitimately happen in the case of a speculative insertion that aborts, so we'd better not flag it as corruption after all. --- contrib/amcheck/verify_heapam.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c index ce3114f4e6..f50ee9d29f 100644 --- a/contrib/amcheck/verify_heapam.c +++ b/contrib/amcheck/verify_heapam.c @@ -1032,8 +1032,7 @@ check_tuple_visibility(HeapCheckContext *ctx, bool *xmin_commit_status_ok, switch (get_xid_status(xmin, ctx, &xmin_status)) { case XID_INVALID: - report_corruption(ctx, - pstrdup("xmin is invalid")); + /* Could be the result of a speculative insertion that aborted. */ return false; case XID_BOUNDS_OK: *xmin_commit_status_ok = true; -- 2.37.1 (Apple Git-137.1) ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. 2023-03-27 17:17 Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> 2023-03-27 18:34 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Peter Geoghegan <[email protected]> 2023-03-27 20:17 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> @ 2023-03-27 20:51 ` Peter Geoghegan <[email protected]> 2023-03-28 20:28 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Peter Geoghegan @ 2023-03-27 20:51 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Mon, Mar 27, 2023 at 1:17 PM Robert Haas <[email protected]> wrote: > Patch attached. This is fine, as far as it goes. Obviously it fixes the immediate problem. > > I don't think that it's particularly likely that having refined > > aborted speculative insertion amcheck coverage will make a critical > > difference to any user, at any time. But "amcheck as documentation of > > the on-disk format" is reason enough to have it. > > Sure, if someone feels like writing the code. I have to admit that I > have mixed feelings about this whole direction. In concept, I agree > with you entirely: a fringe benefit of having checks that tell us > whether or not a page is valid is that it helps to make clear what > page states we think are valid. I don't think that it's a fringe benefit; it's just not necessarily of direct benefit to amcheck users. Before the HOT chain validation patch went in, it was unclear whether certain conceivable on-disk states should constitute corruption. In particular, it wasn't clear to anybody whether or not it was okay for an LP_REDIRECT to point to an LP_DEAD until recently (and probably other things besides that). I don't think that we should assume that the easy part is abstractly defining corruption, while the hard part is writing the tool to check for the corruption. Sometimes it is, but I think that it's often the other way around. > In practice, however, the point you > raise in your first sentence weighs awfully heavily with me. Spending > a lot of energy on checks that are unlikely to catch practical > problems feels like it may not be the best use of time. That definitely could be true, but I don't think that it's terribly much extra effort in most cases. > I'm not sure > exactly where to draw the line, but it seems highly likely to be that > there are things we could deduce about the page that wouldn't be worth > the effort. For example, would we bother checking that a tuple with an > in-progress xmin does not have a smaller natts value than a tuple with > a committed xmin? Or that natts values are non-decreasing across a HOT > chain? I suspect there are even more obscure examples of things that > should be true but might not really be worth worrying about in the > code. A related way of looking at it (that I also find appealing) is that it's often easier (far easier) to just have the check, and be done with it. Of course there is bound to be uncertainty about how useful any given check might be; we're looking for something that is theoretically never supposed to happen. Why not just assume that it might matter if it's not costing very much to check for it? This is quite a different mentality than the one we bring to core heapam code, where it's quite natural to just avoid strange corner cases in the on-disk format like the plague. The risk profile is totally different for amcheck code. Within amcheck, I'd rather go too far than not go far enough. -- Peter Geoghegan ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. 2023-03-27 17:17 Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> 2023-03-27 18:34 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Peter Geoghegan <[email protected]> 2023-03-27 20:17 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> 2023-03-27 20:51 ` Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Peter Geoghegan <[email protected]> @ 2023-03-28 20:28 ` Robert Haas <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Robert Haas @ 2023-03-28 20:28 UTC (permalink / raw) To: Peter Geoghegan <[email protected]>; +Cc: pgsql-hackers On Mon, Mar 27, 2023 at 4:52 PM Peter Geoghegan <[email protected]> wrote: > This is fine, as far as it goes. Obviously it fixes the immediate problem. OK, I've committed and back-patched this fix to v14, just like the erroneous commit that created the issue. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 6+ messages in thread
* [PATCH v46 1/7] Make index_concurrently_create_copy more general @ 2026-03-24 18:02 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Álvaro Herrera @ 2026-03-24 18:02 UTC (permalink / raw) Add a 'boolean concurrent' option, and make it work for both cases. Also rename it to index_create_copy. This allows it to be reused for other purposes -- specifically, for REPACK CONCURRENTLY. With the CONCURRENTLY option, REPACK cannot simply swap the heap file and rebuild its indexes. Instead, it needs to build a separate set of indexes (including system catalog entries) *before* the actual swap, to reduce the time AccessExclusiveLock needs to be held for. This approach is different from what CREATE INDEX CONCURRENTLY does. Per a suggestion from Mihail Nikalayeu. Author: Antonin Houska <[email protected]> Discussion: https://postgr.es/m/41104.1754922120@localhost --- src/backend/catalog/index.c | 39 +++++++++++++++++++------------- src/backend/commands/indexcmds.c | 15 +++++++----- src/backend/nodes/makefuncs.c | 9 ++++---- src/include/catalog/index.h | 7 +++--- src/include/nodes/makefuncs.h | 4 +++- 5 files changed, 43 insertions(+), 31 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index d8219b18c48..de7182a85a9 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1289,17 +1289,17 @@ index_create(Relation heapRelation, } /* - * index_concurrently_create_copy + * index_create_copy * - * Create concurrently an index based on the definition of the one provided by - * caller. The index is inserted into catalogs and needs to be built later - * on. This is called during concurrent reindex processing. + * Create an index based on the definition of the one provided by caller. The + * index is inserted into catalogs. If 'concurrently' is TRUE, it needs to be + * built later on; otherwise it's built immediately. * * "tablespaceOid" is the tablespace to use for this index. */ Oid -index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, - Oid tablespaceOid, const char *newName) +index_create_copy(Relation heapRelation, bool concurrently, + Oid oldIndexId, Oid tablespaceOid, const char *newName) { Relation indexRelation; IndexInfo *oldInfo, @@ -1318,6 +1318,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, List *indexColNames = NIL; List *indexExprs = NIL; List *indexPreds = NIL; + int flags = 0; indexRelation = index_open(oldIndexId, RowExclusiveLock); @@ -1328,7 +1329,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, * Concurrent build of an index with exclusion constraints is not * supported. */ - if (oldInfo->ii_ExclusionOps != NULL) + if (oldInfo->ii_ExclusionOps != NULL && concurrently) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("concurrent index creation for exclusion constraints is not supported"))); @@ -1384,9 +1385,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, } /* - * Build the index information for the new index. Note that rebuild of - * indexes with exclusion constraints is not supported, hence there is no - * need to fill all the ii_Exclusion* fields. + * Build the index information for the new index. */ newInfo = makeIndexInfo(oldInfo->ii_NumIndexAttrs, oldInfo->ii_NumIndexKeyAttrs, @@ -1395,10 +1394,13 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, indexPreds, oldInfo->ii_Unique, oldInfo->ii_NullsNotDistinct, - false, /* not ready for inserts */ - true, + !concurrently, /* isready */ + concurrently, /* concurrent */ indexRelation->rd_indam->amsummarizing, - oldInfo->ii_WithoutOverlaps); + oldInfo->ii_WithoutOverlaps, + oldInfo->ii_ExclusionOps, + oldInfo->ii_ExclusionProcs, + oldInfo->ii_ExclusionStrats); /* * Extract the list of column names and the column numbers for the new @@ -1436,6 +1438,9 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, stattargets[i].isnull = isnull; } + if (concurrently) + flags = INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT; + /* * Now create the new index. * @@ -1459,7 +1464,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, indcoloptions->values, stattargets, reloptionsDatum, - INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT, + flags, 0, true, /* allow table to be a system catalog? */ false, /* is_internal? */ @@ -2453,7 +2458,8 @@ BuildIndexInfo(Relation index) indexStruct->indisready, false, index->rd_indam->amsummarizing, - indexStruct->indisexclusion && indexStruct->indisunique); + indexStruct->indisexclusion && indexStruct->indisunique, + NULL, NULL, NULL); /* fill in attribute numbers */ for (i = 0; i < numAtts; i++) @@ -2513,7 +2519,8 @@ BuildDummyIndexInfo(Relation index) indexStruct->indisready, false, index->rd_indam->amsummarizing, - indexStruct->indisexclusion && indexStruct->indisunique); + indexStruct->indisexclusion && indexStruct->indisunique, + NULL, NULL, NULL); /* fill in attribute numbers */ for (i = 0; i < numAtts; i++) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index dd593ccbc1c..83edab38760 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -244,7 +244,8 @@ CheckIndexCompatible(Oid oldId, */ indexInfo = makeIndexInfo(numberOfAttributes, numberOfAttributes, accessMethodId, NIL, NIL, false, false, - false, false, amsummarizing, isWithoutOverlaps); + false, false, amsummarizing, isWithoutOverlaps, + NULL, NULL, NULL); typeIds = palloc_array(Oid, numberOfAttributes); collationIds = palloc_array(Oid, numberOfAttributes); opclassIds = palloc_array(Oid, numberOfAttributes); @@ -931,7 +932,8 @@ DefineIndex(ParseState *pstate, !concurrent, concurrent, amissummarizing, - stmt->iswithoutoverlaps); + stmt->iswithoutoverlaps, + NULL, NULL, NULL); typeIds = palloc_array(Oid, numberOfAttributes); collationIds = palloc_array(Oid, numberOfAttributes); @@ -3989,10 +3991,11 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein tablespaceid = indexRel->rd_rel->reltablespace; /* Create new index definition based on given index */ - newIndexId = index_concurrently_create_copy(heapRel, - idx->indexId, - tablespaceid, - concurrentName); + newIndexId = index_create_copy(heapRel, + true, + idx->indexId, + tablespaceid, + concurrentName); /* * Now open the relation of the new index, a session-level lock is diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c index 3cd35c5c457..8d23aa917e5 100644 --- a/src/backend/nodes/makefuncs.c +++ b/src/backend/nodes/makefuncs.c @@ -834,7 +834,8 @@ IndexInfo * makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions, List *predicates, bool unique, bool nulls_not_distinct, bool isready, bool concurrent, bool summarizing, - bool withoutoverlaps) + bool withoutoverlaps, Oid *exclusion_ops, Oid *exclusion_procs, + uint16 *exclusion_strats) { IndexInfo *n = makeNode(IndexInfo); @@ -863,9 +864,9 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions, n->ii_PredicateState = NULL; /* exclusion constraints */ - n->ii_ExclusionOps = NULL; - n->ii_ExclusionProcs = NULL; - n->ii_ExclusionStrats = NULL; + n->ii_ExclusionOps = exclusion_ops; + n->ii_ExclusionProcs = exclusion_procs; + n->ii_ExclusionStrats = exclusion_strats; /* speculative inserts */ n->ii_UniqueOps = NULL; diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index 36b70689254..56a064ef444 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -101,10 +101,9 @@ extern Oid index_create(Relation heapRelation, #define INDEX_CONSTR_CREATE_REMOVE_OLD_DEPS (1 << 4) #define INDEX_CONSTR_CREATE_WITHOUT_OVERLAPS (1 << 5) -extern Oid index_concurrently_create_copy(Relation heapRelation, - Oid oldIndexId, - Oid tablespaceOid, - const char *newName); +extern Oid index_create_copy(Relation heapRelation, bool concurrently, + Oid oldIndexId, Oid tablespaceOid, + const char *newName); extern void index_concurrently_build(Oid heapRelationId, Oid indexRelationId); diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h index bf54d39feb0..40ec249a7a1 100644 --- a/src/include/nodes/makefuncs.h +++ b/src/include/nodes/makefuncs.h @@ -99,7 +99,9 @@ extern IndexInfo *makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions, List *predicates, bool unique, bool nulls_not_distinct, bool isready, bool concurrent, - bool summarizing, bool withoutoverlaps); + bool summarizing, bool withoutoverlaps, + Oid *exclusion_ops, Oid *exclusion_procs, + uint16 *exclusion_strats); extern Node *makeStringConst(char *str, int location); extern DefElem *makeDefElem(char *name, Node *arg, int location); -- 2.47.3 --mdsv5bdgfjjj6d77 Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v46-0002-Add-CONCURRENTLY-option-to-REPACK-command.patch" ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-03-24 18:02 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-03-27 17:17 Re: pgsql: amcheck: Fix verify_heapam for tuples where xmin or xmax is 0. Robert Haas <[email protected]> 2023-03-27 18:34 ` Peter Geoghegan <[email protected]> 2023-03-27 20:17 ` Robert Haas <[email protected]> 2023-03-27 20:51 ` Peter Geoghegan <[email protected]> 2023-03-28 20:28 ` Robert Haas <[email protected]> 2026-03-24 18:02 [PATCH v46 1/7] Make index_concurrently_create_copy more general Álvaro Herrera <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox