On Tue, Nov 26, 2024 at 1:58 AM Vallimaharajan G <
[email protected]> wrote:
>
> Hi Developers,
> We have discovered a bug in the parallel_vacuum_reset_dead_items
function in PG v17.2. Specifically:
>
> TidStoreDestroy(dead_items) frees the dead_items pointer.
> The pointer is reinitialized using TidStoreCreateShared().
> However, the code later accesses the freed pointer instead of the newly
reinitialized pvs->dead_items, as seen in these lines:
>
> pvs->shared->dead_items_dsa_handle =
dsa_get_handle(TidStoreGetDSA(dead_items));
> pvs->shared->dead_items_handle = TidStoreGetHandle(dead_items);
Thanks for the report! I don't see any immediate evidence of deleterious
effects, but it's still sloppy. To reduce risk going forward, I think we
should always access this pointer via the struct rather than a separate
copy, quick attempt attached.
(BTW, it's normally discouraged to cross-post to different lists. Either
one is fine in this case.)
--
John Naylor
Amazon Web Services
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 793bd33cb4..667b1c54e1 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2917,8 +2917,6 @@ dead_items_add(LVRelState *vacrel, BlockNumber blkno, OffsetNumber *offsets,
static void
dead_items_reset(LVRelState *vacrel)
{
- TidStore *dead_items = vacrel->dead_items;
-
if (ParallelVacuumIsActive(vacrel))
{
parallel_vacuum_reset_dead_items(vacrel->pvs);
@@ -2926,7 +2924,7 @@ dead_items_reset(LVRelState *vacrel)
}
/* Recreate the tidstore with the same max_bytes limitation */
- TidStoreDestroy(dead_items);
+ TidStoreDestroy(vacrel->dead_items);
vacrel->dead_items = TidStoreCreateLocal(vacrel->dead_items_info->max_bytes, true);
/* Reset the counter */
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 4fd6574e12..4b0669b1e6 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -247,7 +247,6 @@ parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes,
ParallelVacuumState *pvs;
ParallelContext *pcxt;
PVShared *shared;
- TidStore *dead_items;
PVIndStats *indstats;
BufferUsage *buffer_usage;
WalUsage *wal_usage;
@@ -378,11 +377,10 @@ parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes,
shared->dead_items_info.max_bytes = vac_work_mem * 1024L;
/* Prepare DSA space for dead items */
- dead_items = TidStoreCreateShared(shared->dead_items_info.max_bytes,
- LWTRANCHE_PARALLEL_VACUUM_DSA);
- pvs->dead_items = dead_items;
- shared->dead_items_handle = TidStoreGetHandle(dead_items);
- shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(dead_items));
+ pvs->dead_items = TidStoreCreateShared(shared->dead_items_info.max_bytes,
+ LWTRANCHE_PARALLEL_VACUUM_DSA);
+ shared->dead_items_handle = TidStoreGetHandle(pvs->dead_items);
+ shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(pvs->dead_items));
/* Use the same buffer size for all workers */
shared->ring_nbuffers = GetAccessStrategyBufferCount(bstrategy);
@@ -474,7 +472,6 @@ parallel_vacuum_get_dead_items(ParallelVacuumState *pvs, VacDeadItemsInfo **dead
void
parallel_vacuum_reset_dead_items(ParallelVacuumState *pvs)
{
- TidStore *dead_items = pvs->dead_items;
VacDeadItemsInfo *dead_items_info = &(pvs->shared->dead_items_info);
/*
@@ -482,13 +479,13 @@ parallel_vacuum_reset_dead_items(ParallelVacuumState *pvs)
* operating system. Then we recreate the tidstore with the same max_bytes
* limitation we just used.
*/
- TidStoreDestroy(dead_items);
+ TidStoreDestroy(pvs->dead_items);
pvs->dead_items = TidStoreCreateShared(dead_items_info->max_bytes,
LWTRANCHE_PARALLEL_VACUUM_DSA);
/* Update the DSA pointer for dead_items to the new one */
- pvs->shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(dead_items));
- pvs->shared->dead_items_handle = TidStoreGetHandle(dead_items);
+ pvs->shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(pvs->dead_items));
+ pvs->shared->dead_items_handle = TidStoreGetHandle(pvs->dead_items);
/* Reset the counter */
dead_items_info->num_items = 0;
