Обсуждение: repack: fix uninitialized DecodingWorkerShared.initialized

Hi,

I have not reviewed the repack-related patches before. Recently, I started trying to understand how repack works and
tracethrough 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
codeexplicitly 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
structexplicitly, and then initialize the non-zero fields afterwards. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Chao Li <li.evan.chao@gmail.com> wrote:

> I have not reviewed the repack-related patches before. Recently, I started trying to understand how repack works and
tracethrough 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
fieldsof “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
structexplicitly, 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

> On Apr 15, 2026, at 23:07, Antonin Houska <ah@cybertec.at> wrote:
>
> Chao Li <li.evan.chao@gmail.com> wrote:
>
>> I have not reviewed the repack-related patches before. Recently, I started trying to understand how repack works and
tracethrough 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
fieldsof “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
structexplicitly, 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/

On Thu, Apr 16, 2026 at 2:17 AM Chao Li <li.evan.chao@gmail.com> 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
codeexplicitly 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).