public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4] fix HOT tuples while scanning for index builds
4+ messages / 3 participants
[nested] [flat]
* [PATCH v4] fix HOT tuples while scanning for index builds
@ 2020-08-12 18:02 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Alvaro Herrera @ 2020-08-12 18:02 UTC (permalink / raw)
---
src/backend/access/heap/heapam_handler.c | 20 ++++++++++++++++++++
src/backend/access/heap/pruneheap.c | 5 +++--
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 267a6ee25a..ba44e30035 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -1324,6 +1324,12 @@ heapam_index_build_range_scan(Relation heapRelation,
* buffer continuously while visiting the page, so no pruning
* operation can occur either.
*
+ * In cases with only ShareUpdateExclusiveLock on the table, it's
+ * possible for some HOT tuples to appear that we didn't know about
+ * when we first read the page. To handle that case, we re-obtain the
+ * list of root offsets when a HOT tuple points to a root item that we
+ * don't know about.
+ *
* Also, although our opinions about tuple liveness could change while
* we scan the page (due to concurrent transaction commits/aborts),
* the chain root locations won't, so this info doesn't need to be
@@ -1625,6 +1631,20 @@ heapam_index_build_range_scan(Relation heapRelation,
offnum = ItemPointerGetOffsetNumber(&heapTuple->t_self);
+ /*
+ * If a HOT tuple points to a root that we don't know
+ * about, obtain root items afresh. If that still fails,
+ * report it as corruption.
+ */
+ if (root_offsets[offnum - 1] == InvalidOffsetNumber)
+ {
+ Page page = BufferGetPage(hscan->rs_cbuf);
+
+ LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_SHARE);
+ heap_get_root_tuples(page, root_offsets);
+ LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ }
+
if (!OffsetNumberIsValid(root_offsets[offnum - 1]))
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 256df4de10..7e3d44dfd6 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -732,7 +732,7 @@ heap_page_prune_execute(Buffer buffer,
* root_offsets[k - 1] = j.
*
* The passed-in root_offsets array must have MaxHeapTuplesPerPage entries.
- * We zero out all unused entries.
+ * Unused entries are filled with InvalidOffsetNumber (zero).
*
* The function must be called with at least share lock on the buffer, to
* prevent concurrent prune operations.
@@ -747,7 +747,8 @@ heap_get_root_tuples(Page page, OffsetNumber *root_offsets)
OffsetNumber offnum,
maxoff;
- MemSet(root_offsets, 0, MaxHeapTuplesPerPage * sizeof(OffsetNumber));
+ MemSet(root_offsets, InvalidOffsetNumber,
+ MaxHeapTuplesPerPage * sizeof(OffsetNumber));
maxoff = PageGetMaxOffsetNumber(page);
for (offnum = FirstOffsetNumber; offnum <= maxoff; offnum = OffsetNumberNext(offnum))
--
2.20.1
--opJtzjQTFsWo+cga--
^ permalink raw reply [nested|flat] 4+ messages in thread
* null iv parameter passed to combo_init()
@ 2022-01-08 00:32 Zhihong Yu <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Zhihong Yu @ 2022-01-08 00:32 UTC (permalink / raw)
To: pgsql-hackers
Hi,
In contrib/pgcrypto/pgcrypto.c :
err = px_combo_init(c, (uint8 *) VARDATA_ANY(key), klen, NULL, 0);
Note: NULL is passed as iv.
When combo_init() is called,
if (ivlen > ivs)
memcpy(ivbuf, iv, ivs);
else
memcpy(ivbuf, iv, ivlen);
It seems we need to consider the case of null being passed as iv for
memcpy() because of this:
/usr/include/string.h:44:28: note: nonnull attribute specified here
What do you think of the following patch ?
Cheers
Attachments:
[application/octet-stream] memcpy-null-iv.patch (503B, ../../CALNJ-vSBb2Ees=KB0frYBh7foK-QNZMbK7Vz66bUJa09D+CHAw@mail.gmail.com/3-memcpy-null-iv.patch)
download | inline diff:
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index 4205e9c3ef..d3e057a453 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -198,10 +198,13 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
if (ivs > 0)
{
ivbuf = palloc0(ivs);
- if (ivlen > ivs)
- memcpy(ivbuf, iv, ivs);
- else
- memcpy(ivbuf, iv, ivlen);
+ if (iv != NULL)
+ {
+ if (ivlen > ivs)
+ memcpy(ivbuf, iv, ivs);
+ else
+ memcpy(ivbuf, iv, ivlen);
+ }
}
if (klen > ks)
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: null iv parameter passed to combo_init()
@ 2022-01-09 01:52 Noah Misch <[email protected]>
parent: Zhihong Yu <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Noah Misch @ 2022-01-09 01:52 UTC (permalink / raw)
To: Zhihong Yu <[email protected]>; +Cc: pgsql-hackers
On Fri, Jan 07, 2022 at 04:32:01PM -0800, Zhihong Yu wrote:
> In contrib/pgcrypto/pgcrypto.c :
>
> err = px_combo_init(c, (uint8 *) VARDATA_ANY(key), klen, NULL, 0);
>
> Note: NULL is passed as iv.
>
> When combo_init() is called,
>
> if (ivlen > ivs)
> memcpy(ivbuf, iv, ivs);
> else
> memcpy(ivbuf, iv, ivlen);
>
> It seems we need to consider the case of null being passed as iv for
> memcpy() because of this:
>
> /usr/include/string.h:44:28: note: nonnull attribute specified here
I agree it's time to fix cases like this, given
https://postgr.es/m/flat/[email protected]. However,
it should be one patch fixing all (or at least many) of them.
> --- a/contrib/pgcrypto/px.c
> +++ b/contrib/pgcrypto/px.c
> @@ -198,10 +198,13 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
> if (ivs > 0)
> {
> ivbuf = palloc0(ivs);
> - if (ivlen > ivs)
> - memcpy(ivbuf, iv, ivs);
> - else
> - memcpy(ivbuf, iv, ivlen);
> + if (iv != NULL)
> + {
> + if (ivlen > ivs)
> + memcpy(ivbuf, iv, ivs);
> + else
> + memcpy(ivbuf, iv, ivlen);
> + }
> }
If someone were to pass NULL iv with nonzero ivlen, that will silently
malfunction. I'd avoid that risk by writing this way:
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -202,3 +202,3 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
memcpy(ivbuf, iv, ivs);
- else
+ else if (ivlen > 0)
memcpy(ivbuf, iv, ivlen);
That also gives the compiler an additional optimization strategy.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: null iv parameter passed to combo_init()
@ 2022-01-09 02:52 Zhihong Yu <[email protected]>
parent: Noah Misch <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Zhihong Yu @ 2022-01-09 02:52 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: pgsql-hackers
On Sat, Jan 8, 2022 at 5:52 PM Noah Misch <[email protected]> wrote:
> On Fri, Jan 07, 2022 at 04:32:01PM -0800, Zhihong Yu wrote:
> > In contrib/pgcrypto/pgcrypto.c :
> >
> > err = px_combo_init(c, (uint8 *) VARDATA_ANY(key), klen, NULL, 0);
> >
> > Note: NULL is passed as iv.
> >
> > When combo_init() is called,
> >
> > if (ivlen > ivs)
> > memcpy(ivbuf, iv, ivs);
> > else
> > memcpy(ivbuf, iv, ivlen);
> >
> > It seems we need to consider the case of null being passed as iv for
> > memcpy() because of this:
> >
> > /usr/include/string.h:44:28: note: nonnull attribute specified here
>
> I agree it's time to fix cases like this, given
> https://postgr.es/m/flat/[email protected].
> However,
> it should be one patch fixing all (or at least many) of them.
> > --- a/contrib/pgcrypto/px.c
> > +++ b/contrib/pgcrypto/px.c
> > @@ -198,10 +198,13 @@ combo_init(PX_Combo *cx, const uint8 *key,
> unsigned klen,
> > if (ivs > 0)
> > {
> > ivbuf = palloc0(ivs);
> > - if (ivlen > ivs)
> > - memcpy(ivbuf, iv, ivs);
> > - else
> > - memcpy(ivbuf, iv, ivlen);
> > + if (iv != NULL)
> > + {
> > + if (ivlen > ivs)
> > + memcpy(ivbuf, iv, ivs);
> > + else
> > + memcpy(ivbuf, iv, ivlen);
> > + }
> > }
>
> If someone were to pass NULL iv with nonzero ivlen, that will silently
>
Hi,
If iv is NULL, none of the memcpy() would be called (based on my patch).
Can you elaborate your suggestion in more detail ?
Patch v2 is attached, covering more files.
Since the referenced email was old, line numbers have changed.
It would be nice if an up-to-date list is provided in case more places
should be changed.
Cheers
> malfunction. I'd avoid that risk by writing this way:
>
> --- a/contrib/pgcrypto/px.c
> +++ b/contrib/pgcrypto/px.c
> @@ -202,3 +202,3 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned
> klen,
> memcpy(ivbuf, iv, ivs);
> - else
> + else if (ivlen > 0)
> memcpy(ivbuf, iv, ivlen);
>
> That also gives the compiler an additional optimization strategy.
>
Attachments:
[application/octet-stream] 0002-memcpy-null.patch (2.3K, ../../CALNJ-vTh9BXARHg3iaCAzr97Fws2W5aR6nRPJfT4u3VdKDzRTQ@mail.gmail.com/3-0002-memcpy-null.patch)
download | inline diff:
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index 4205e9c3ef..d3e057a453 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -198,10 +198,13 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
if (ivs > 0)
{
ivbuf = palloc0(ivs);
- if (ivlen > ivs)
- memcpy(ivbuf, iv, ivs);
- else
- memcpy(ivbuf, iv, ivlen);
+ if (iv != NULL)
+ {
+ if (ivlen > ivs)
+ memcpy(ivbuf, iv, ivs);
+ else
+ memcpy(ivbuf, iv, ivlen);
+ }
}
if (klen > ks)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index de787c3d37..eca1e5dd0e 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -294,7 +294,7 @@ TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
* sub-XIDs and all of the XIDs for which we're adjusting clog should be
* on the same page. Check those conditions, too.
*/
- if (all_xact_same_page && xid == MyProc->xid &&
+ if (subxids != NULL && all_xact_same_page && xid == MyProc->xid &&
nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT &&
nsubxids == MyProc->subxidStatus.count &&
memcmp(subxids, MyProc->subxids.xids,
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c9516e03fa..3d5ca89d42 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5329,7 +5329,7 @@ SerializeTransactionState(Size maxsize, char *start_address)
* of our own, we can just pass along the information that was passed to
* us.
*/
- if (nParallelCurrentXids > 0)
+ if (nParallelCurrentXids > 0 && ParallelCurrentXids != NULL)
{
result->nParallelCurrentXids = nParallelCurrentXids;
memcpy(&result->parallelCurrentXids[0], ParallelCurrentXids,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e5cf1bde13..ae10c55711 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1182,7 +1182,10 @@ DefineIndex(Oid relationId,
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
nparts);
- memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ if (partdesc->oids != NULL)
+ {
+ memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ }
parentDesc = RelationGetDescr(rel);
opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-01-09 02:52 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-08-12 18:02 [PATCH v4] fix HOT tuples while scanning for index builds Alvaro Herrera <[email protected]>
2022-01-08 00:32 null iv parameter passed to combo_init() Zhihong Yu <[email protected]>
2022-01-09 01:52 ` Re: null iv parameter passed to combo_init() Noah Misch <[email protected]>
2022-01-09 02:52 ` Re: null iv parameter passed to combo_init() Zhihong Yu <[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