public inbox for [email protected]
help / color / mirror / Atom feedrepack: fix uninitialized DecodingWorkerShared.initialized
4+ messages / 3 participants
[nested] [flat]
* repack: fix uninitialized DecodingWorkerShared.initialized
@ 2026-04-15 10:22 Chao Li <[email protected]>
0 siblings, 2 replies; 4+ messages in thread
From: Chao Li @ 2026-04-15 10:22 UTC (permalink / raw)
To: Postgres hackers <[email protected]>
Hi,
I have not reviewed the repack-related patches before. Recently, I started trying to understand how repack works and trace through the code.
While tracing start_repack_decoding_worker(), I noticed something suspicious:
```
seg = dsm_create(size, 0);
shared = (DecodingWorkerShared *) dsm_segment_address(seg);
shared->lsn_upto = InvalidXLogRecPtr;
shared->done = false;
SharedFileSetInit(&shared->sfs, seg);
shared->last_exported = -1;
SpinLockInit(&shared->mutex);
shared->dbid = MyDatabaseId;
```
Here, the code creates a shared-memory segment and lets “shared" point to that memory. It then initializes some fields of “shared". However, later code reads shared->initialized, but this field was not initialized:
```
for (;;)
{
bool initialized;
SpinLockAcquire(&shared->mutex);
initialized = shared->initialized;
SpinLockRelease(&shared->mutex);
if (initialized)
break;
ConditionVariableSleep(&shared->cv, WAIT_EVENT_REPACK_WORKER_EXPORT);
}
```
I checked the code of dsm_create(), and I did not see anything showing that the created shared-memory segment is zeroed.
This is actually just an eyeball finding. From my tracing on my MacBook, after shared = (DecodingWorkerShared *) dsm_segment_address(seg);, the memory always seemed to be zeroed, so I may have missed something. But given that the code explicitly initializes several other fields, it seems better not to rely on that implicitly.
For the fix, since start_repack_decoding_worker() is not on a hot path, I think it is fine to zero the whole shared struct explicitly, and then initialize the non-zero fields afterwards.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v1-0001-repack-zero-initialize-DecodingWorkerShared.patch (1010B, 2-v1-0001-repack-zero-initialize-DecodingWorkerShared.patch)
download | inline diff:
From b9c22b95492bff66a511159ceffd37d6394cc5b1 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Wed, 15 Apr 2026 16:46:20 +0800
Subject: [PATCH v1] repack: zero-initialize DecodingWorkerShared
Author: Chao Li <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/commands/repack.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 58e3867246f..058b39a67ad 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -3311,8 +3311,7 @@ start_repack_decoding_worker(Oid relid)
BUFFERALIGN(REPACK_ERROR_QUEUE_SIZE);
seg = dsm_create(size, 0);
shared = (DecodingWorkerShared *) dsm_segment_address(seg);
- shared->lsn_upto = InvalidXLogRecPtr;
- shared->done = false;
+ memset(shared, 0, sizeof(DecodingWorkerShared));
SharedFileSetInit(&shared->sfs, seg);
shared->last_exported = -1;
SpinLockInit(&shared->mutex);
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: repack: fix uninitialized DecodingWorkerShared.initialized
@ 2026-04-15 15:07 Antonin Houska <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 1 reply; 4+ messages in thread
From: Antonin Houska @ 2026-04-15 15:07 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]>
Chao Li <[email protected]> wrote:
> I have not reviewed the repack-related patches before. Recently, I started trying to understand how repack works and trace through the code.
Thanks, I appreciate that!
> While tracing start_repack_decoding_worker(), I noticed something suspicious:
>
> ```
> seg = dsm_create(size, 0);
> shared = (DecodingWorkerShared *) dsm_segment_address(seg);
> shared->lsn_upto = InvalidXLogRecPtr;
> shared->done = false;
> SharedFileSetInit(&shared->sfs, seg);
> shared->last_exported = -1;
> SpinLockInit(&shared->mutex);
> shared->dbid = MyDatabaseId;
> ```
>
> Here, the code creates a shared-memory segment and lets “shared" point to that memory. It then initializes some fields of “shared". However, later code reads shared->initialized, but this field was not initialized:
The problem was noticed earlier this week and I already posted a fix [1].
> For the fix, since start_repack_decoding_worker() is not on a hot path, I think it is fine to zero the whole shared struct explicitly, and then initialize the non-zero fields afterwards.
Although not strongly, I prefer setting individual fields explicitly.
[1] https://www.postgresql.org/message-id/182883.1776073323%40localhost
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: repack: fix uninitialized DecodingWorkerShared.initialized
@ 2026-04-17 03:41 Chao Li <[email protected]>
parent: Antonin Houska <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Chao Li @ 2026-04-17 03:41 UTC (permalink / raw)
To: Antonin Houska <[email protected]>; +Cc: Postgres hackers <[email protected]>
> On Apr 15, 2026, at 23:07, Antonin Houska <[email protected]> wrote:
>
> Chao Li <[email protected]> wrote:
>
>> I have not reviewed the repack-related patches before. Recently, I started trying to understand how repack works and trace through the code.
>
> Thanks, I appreciate that!
>
>> While tracing start_repack_decoding_worker(), I noticed something suspicious:
>>
>> ```
>> seg = dsm_create(size, 0);
>> shared = (DecodingWorkerShared *) dsm_segment_address(seg);
>> shared->lsn_upto = InvalidXLogRecPtr;
>> shared->done = false;
>> SharedFileSetInit(&shared->sfs, seg);
>> shared->last_exported = -1;
>> SpinLockInit(&shared->mutex);
>> shared->dbid = MyDatabaseId;
>> ```
>>
>> Here, the code creates a shared-memory segment and lets “shared" point to that memory. It then initializes some fields of “shared". However, later code reads shared->initialized, but this field was not initialized:
>
> The problem was noticed earlier this week and I already posted a fix [1].
>
>> For the fix, since start_repack_decoding_worker() is not on a hot path, I think it is fine to zero the whole shared struct explicitly, and then initialize the non-zero fields afterwards.
>
> Although not strongly, I prefer setting individual fields explicitly.
>
>
> [1] https://www.postgresql.org/message-id/182883.1776073323%40localhost
>
> --
> Antonin Houska
> Web: https://www.cybertec-postgresql.com
I saw 05c401d5786a05ea630e884ffa492aa01683d15b has fixed this issue, so this patch is no longer needed.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: repack: fix uninitialized DecodingWorkerShared.initialized
@ 2026-04-17 10:27 Thomas Munro <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Munro @ 2026-04-17 10:27 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]>
On Thu, Apr 16, 2026 at 2:17 AM Chao Li <[email protected]> wrote:
> I checked the code of dsm_create(), and I did not see anything showing that the created shared-memory segment is zeroed.
>
> This is actually just an eyeball finding. From my tracing on my MacBook, after shared = (DecodingWorkerShared *) dsm_segment_address(seg);, the memory always seemed to be zeroed, so I may have missed something. But given that the code explicitly initializes several other fields, it seems better not to rely on that implicitly.
For the record, that depends on whether you get a newly allocated
shared memory segment from the operating system, or recycled shared
memory reserved at startup with the min_dynamic_shared_memory setting
(as shown in Alexander Lakhin's reproducer).
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-04-17 10:27 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-15 10:22 repack: fix uninitialized DecodingWorkerShared.initialized Chao Li <[email protected]>
2026-04-15 15:07 ` Antonin Houska <[email protected]>
2026-04-17 03:41 ` Chao Li <[email protected]>
2026-04-17 10:27 ` Thomas Munro <[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