public inbox for [email protected]
help / color / mirror / Atom feedRe: BUG: Postgres 14 + vacuum_defer_cleanup_age + FOR UPDATE + UPDATE
3+ messages / 3 participants
[nested] [flat]
* Re: BUG: Postgres 14 + vacuum_defer_cleanup_age + FOR UPDATE + UPDATE
@ 2023-01-31 22:38 Andres Freund <[email protected]>
2023-02-01 00:23 ` Re: BUG: Postgres 14 + vacuum_defer_cleanup_age + FOR UPDATE + UPDATE Matthias van de Meent <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Andres Freund @ 2023-01-31 22:38 UTC (permalink / raw)
To: Matthias van de Meent <[email protected]>; +Cc: Michail Nikolaev <[email protected]>; Thomas Munro <[email protected]>; Peter Geoghegan <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Mark Dilger <[email protected]>
Hi,
On 2023-01-31 15:05:17 +0100, Matthias van de Meent wrote:
> On Mon, 30 Jan 2023 at 21:19, Andres Freund <[email protected]> wrote:
> > In an earlier, not posted, version I had an vacuum_defer_cleanup_age specific
> > helper function for this, but it seems likely we'll need it in other places
> > too. So I named it TransactionIdRetreatSafely(). I made it accept the xid by
> > pointer, as the line lengths / repetition otherwise end up making it hard to
> > read the code. For now I have TransactionIdRetreatSafely() be private to
> > procarray.c, but I expect we'll have to change that eventually.
>
> If TransactionIdRetreatSafely will be exposed outside procarray.c,
> then I think the xid pointer should be replaced with normal
> arguments/returns; both for parity with TransactionIdRetreatedBy
That's why I named one version *Retreat the other Retreated :)
I think it'll make the code easier to read in the other places too, the
variable names / function names in this space are uncomfortably long to
fit into 78chars..., particularly when there's two references to the
same variable in the same line.
> and to remove this memory store dependency in this hot code path.
I doubt that matters much here and the places it's going to be used
in. And presumably the compiler will inline it anyway. I'd probably make
it a static inline in the header too.
What's making me hesitate about exposing it is that it's quite easy to
get things wrong by using a wrong fxid or such.
> > + /*
> > + * FIXME, doubtful this is the best fix.
> > + *
> > + * Can't represent the 32bit xid as a 64bit xid, as it's before fxid
> > + * 0. Represent it as an xid from the future instead.
> > + */
> > + if (epoch == 0)
> > + return FullTransactionIdFromEpochAndXid(0, xid);
>
> Shouldn't this be an error condition instead, as this XID should not
> be able to appear?
If you mean error in the sense of ERROR, no, I don't think so. That code
tries hard to be able to check many tuples in a row. And if we were to
error out here, we'd not able to do that. We should still report those
tuples as corrupt, fwiw.
The reason this path is hit is that a test intentionally corrupts some
xids. So the path is reachable and we need to cope somehow.
I'm not really satisfied with this fix either - I mostly wanted to
include something sufficient to prevent assertion failures.
I had hoped that Mark would look at the amcheck bits and come up with
more complete fixes.
> on 0004:
>
> > - '0xffffffffffffffff'::xid8,
> > - '-1'::xid8;
> > + '0xefffffffffffffff'::xid8,
> > + '0'::xid8;
>
> The 0xFF... usages were replaced with "0xEFFF...". Shouldn't we also
> test on 0xffff_fffE_ffff_ffff to test for input of our actual max
> value?
Probably a good idea.
> > @@ -326,7 +329,11 @@ parse_snapshot(const char *str, Node *escontext)
> > while (*str != '\0')
> > {
> > /* read next value */
> > - val = FullTransactionIdFromU64(strtou64(str, &endp, 10));
> > + raw_fxid = strtou64(str, &endp, 10);
> > +
> > + val = FullTransactionIdFromU64(raw_fxid);
> > + if (!InFullTransactionIdRange(raw_fxid))
> > + goto bad_format;
>
> With assertions enabled FullTransactionIdFromU64 will assert the
> InFullTransactionIdRange condition, meaning we wouldn't hit the branch
> into bad_format.
> I think these operations should be swapped, as parsing a snapshot
> shouldn't run into assertion failures like this if it can error
> normally.
Yep.
> Maybe this can be added to tests as well?
I'll check. I thought for a bit it'd not work because we'd perform range
checks on the xids, but we don't...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: BUG: Postgres 14 + vacuum_defer_cleanup_age + FOR UPDATE + UPDATE
2023-01-31 22:38 Re: BUG: Postgres 14 + vacuum_defer_cleanup_age + FOR UPDATE + UPDATE Andres Freund <[email protected]>
@ 2023-02-01 00:23 ` Matthias van de Meent <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Matthias van de Meent @ 2023-02-01 00:23 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michail Nikolaev <[email protected]>; Thomas Munro <[email protected]>; Peter Geoghegan <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Mark Dilger <[email protected]>
On Tue, 31 Jan 2023 at 23:48, Andres Freund <[email protected]> wrote:
>
> Hi,
>
> On 2023-01-31 15:05:17 +0100, Matthias van de Meent wrote:
> > If TransactionIdRetreatSafely will be exposed outside procarray.c,
> > then I think the xid pointer should be replaced with normal
> > arguments/returns; both for parity with TransactionIdRetreatedBy
>
> That's why I named one version *Retreat the other Retreated :)
That part I noticed too :) I don't mind either way, I was just
concerned with exposing the function as a prototype, not as an inline
static.
> I think it'll make the code easier to read in the other places too, the
> variable names / function names in this space are uncomfortably long to
> fit into 78chars..., particularly when there's two references to the
> same variable in the same line.
I guess that's true, and once inlined there should indeed be no extra
runtime overhead.
> 78 chars
Didn't we use 80 columns/chars? How did you get to 78? Not that I
can't think of any ways, but none of them stand out to me as obviously
correct.
> > and to remove this memory store dependency in this hot code path.
>
> I doubt that matters much here and the places it's going to be used
> in.
I thought that this was executed while still in ProcArrayLock, but
instead we've released that lock already by the time we're trying to
adjust the horizons, so the 'hot code path' concern is mostly
relieved.
> And presumably the compiler will inline it anyway. I'd probably make
> it a static inline in the header too.
Yes, my concern was based on an extern prototype with private
implementation, as that does prohibit inlining and thus would have a
requirement to push the data to memory (probably only L1, but still
memory).
> What's making me hesitate about exposing it is that it's quite easy to
> get things wrong by using a wrong fxid or such.
I'm less concerned about that when the function is well-documented.
> > > + /*
> > > + * FIXME, doubtful this is the best fix.
> > > + *
> > > + * Can't represent the 32bit xid as a 64bit xid, as it's before fxid
> > > + * 0. Represent it as an xid from the future instead.
> > > + */
> > > + if (epoch == 0)
> > > + return FullTransactionIdFromEpochAndXid(0, xid);
> >
> > Shouldn't this be an error condition instead, as this XID should not
> > be able to appear?
>
> If you mean error in the sense of ERROR, no, I don't think so. That code
> tries hard to be able to check many tuples in a row. And if we were to
> error out here, we'd not able to do that. We should still report those
> tuples as corrupt, fwiw.
>
> The reason this path is hit is that a test intentionally corrupts some
> xids. So the path is reachable and we need to cope somehow.
I see.
Kind regards,
Matthias van de Meent
^ permalink raw reply [nested|flat] 3+ messages in thread
* [PATCH 2/6] Refactor index_concurrently_create_copy() for use with REPACK (CONCURRENTLY).
@ 2025-12-13 18:27 Antonin Houska <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Antonin Houska @ 2025-12-13 18:27 UTC (permalink / raw)
This patch moves the code to index_create_copy() and adds a "concurrently"
parameter so it can be used by 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.
---
src/backend/catalog/index.c | 54 +++++++++++++++++++++++---------
src/backend/commands/cluster.c | 8 ++---
src/backend/commands/indexcmds.c | 6 ++--
src/backend/nodes/makefuncs.c | 9 +++---
src/include/catalog/index.h | 3 ++
src/include/nodes/makefuncs.h | 4 ++-
6 files changed, 57 insertions(+), 27 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index bd77584bc99..60efa77d34f 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1290,15 +1290,32 @@ index_create(Relation heapRelation,
/*
* index_concurrently_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.
- *
- * "tablespaceOid" is the tablespace to use for this index.
+ * Variant of index_create_copy(), called during concurrent reindex
+ * processing.
*/
Oid
index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
Oid tablespaceOid, const char *newName)
+{
+ return index_create_copy(heapRelation, oldIndexId, tablespaceOid, newName,
+ true);
+}
+
+/*
+ * index_create_copy
+ *
+ * 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.
+ *
+ * The actual implementation of index_concurrently_create_copy(), reusable for
+ * other purposes.
+ */
+Oid
+index_create_copy(Relation heapRelation, Oid oldIndexId, Oid tablespaceOid,
+ const char *newName, bool concurrently)
{
Relation indexRelation;
IndexInfo *oldInfo,
@@ -1317,6 +1334,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);
@@ -1327,7 +1345,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")));
@@ -1383,9 +1401,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,
@@ -1394,10 +1410,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
@@ -1435,6 +1454,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.
*
@@ -1458,7 +1480,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? */
@@ -2452,7 +2474,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++)
@@ -2512,7 +2535,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/cluster.c b/src/backend/commands/cluster.c
index 7f772c5c4f8..a2d72ce494d 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -70,8 +70,7 @@ typedef struct
static bool cluster_rel_recheck(RepackCommand cmd, Relation OldHeap,
Oid indexOid, Oid userid, int options);
-static void rebuild_relation(RepackCommand cmd,
- Relation OldHeap, Relation index, bool verbose);
+static void rebuild_relation(Relation OldHeap, Relation index, bool verbose);
static void copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
@@ -415,7 +414,7 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid,
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- rebuild_relation(cmd, OldHeap, index, verbose);
+ rebuild_relation(OldHeap, index, verbose);
/* rebuild_relation closes OldHeap, and index if valid */
out:
@@ -629,8 +628,7 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
* On exit, they are closed, but locks on them are not released.
*/
static void
-rebuild_relation(RepackCommand cmd,
- Relation OldHeap, Relation index, bool verbose)
+rebuild_relation(Relation OldHeap, Relation index, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid accessMethod = OldHeap->rd_rel->relam;
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d9cccb6ac18..d8d8f72a875 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -242,7 +242,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);
@@ -927,7 +928,8 @@ DefineIndex(Oid tableId,
!concurrent,
concurrent,
amissummarizing,
- stmt->iswithoutoverlaps);
+ stmt->iswithoutoverlaps,
+ NULL, NULL, NULL);
typeIds = palloc_array(Oid, numberOfAttributes);
collationIds = palloc_array(Oid, numberOfAttributes);
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index e2d9e9be41a..c5d5a37f514 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 dda95e54903..4bf909078d8 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -99,6 +99,9 @@ extern Oid index_concurrently_create_copy(Relation heapRelation,
Oid oldIndexId,
Oid tablespaceOid,
const char *newName);
+extern Oid index_create_copy(Relation heapRelation, Oid oldIndexId,
+ Oid tablespaceOid, const char *newName,
+ bool concurrently);
extern void index_concurrently_build(Oid heapRelationId,
Oid indexRelationId);
diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h
index 5473ce9a288..9ff7159ff0c 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
--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
filename=v28-0003-Move-conversion-of-a-historic-to-MVCC-snapshot-to-a-.patch
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2025-12-13 18:27 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 22:38 Re: BUG: Postgres 14 + vacuum_defer_cleanup_age + FOR UPDATE + UPDATE Andres Freund <[email protected]>
2023-02-01 00:23 ` Matthias van de Meent <[email protected]>
2025-12-13 18:27 [PATCH 2/6] Refactor index_concurrently_create_copy() for use with REPACK (CONCURRENTLY). Antonin Houska <[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