pg_resetwal: Fix wrong directory in log output

On Tue, Feb 03, 2026 at 01:58:19AM +0000, zhang_tian_chen@163.com wrote:

There is a misuse of macro when we output directory information in
KillExistingWALSummaries(), pg_resetwal.c. Which should be
WALSUMMARYDIR instead of ARCHSTATDIR.

Eh. Good find.
--
Michael

On Feb 3, 2026, at 09:58, zhang_tian_chen@163.com wrote:

Hi hackers,

There is a misuse of macro when we output directory information in KillExistingWALSummaries(), pg_resetwal.c. Which should be WALSUMMARYDIR instead of ARCHSTATDIR.

Best regards,
Tianchen Zhang<v1-0001-Fix-incorrect-directory-macro-in-KillExistingWALS.patch>

Indeed a bug. Looking at the code:

```
/*
* Remove existing WAL summary files
*/
static void
KillExistingWALSummaries(void)
{
DIR *xldir;

xldir = opendir(WALSUMMARYDIR);
if (xldir == NULL)
pg_fatal("could not open directory \"%s\": %m", WALSUMMARYDIR);
...

if (errno)
pg_fatal("could not read directory \"%s\": %m", WALSUMMARYDIR);

if (closedir(xldir))
pg_fatal("could not close directory \"%s\": %m", ARCHSTATDIR); <<=== It should really be WALSUMMARYDIR
}
```

I guess closedir() is hard to fail, that’s why the problem has not been noticed earlier.

The patch is straightforward and looks correct.

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

Hello.

At Tue, 3 Feb 2026 10:13:53 +0800, Chao Li <li.evan.chao@gmail.com> wrote in

The patch is straightforward and looks correct.

Indeed, it does.

However, the scoping of the define symbols in this file seems somewhat
confused.

For example:

static void
KillExistingArchiveStatus(void)
{
#define ARCHSTATDIR XLOGDIR "/archive_status"

This define was presumably intended to be used only within the function,
but since it is a macro, it can also be used from other functions, which
appears to have contributed to this mistake.

It would be better to clearly distinguish between file-scoped symbols
and function-scoped ones. For function-local constants like this,
using a 'static const char[] instead of a macro would make the
intended scope explicit and help prevent similar issues.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Feb 3, 2026, at 10:25, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

Hello.

At Tue, 3 Feb 2026 10:13:53 +0800, Chao Li <li.evan.chao@gmail.com> wrote in

The patch is straightforward and looks correct.

Indeed, it does.

However, the scoping of the define symbols in this file seems somewhat
confused.

For example:

static void
KillExistingArchiveStatus(void)
{
#define ARCHSTATDIR XLOGDIR "/archive_status"

This define was presumably intended to be used only within the function,
but since it is a macro, it can also be used from other functions, which
appears to have contributed to this mistake.

It would be better to clearly distinguish between file-scoped symbols
and function-scoped ones. For function-local constants like this,
using a 'static const char[] instead of a macro would make the
intended scope explicit and help prevent similar issues.

Or we can just undef the macros before closing the functions.

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

At Tue, 3 Feb 2026 11:46:03 +0800, Chao Li <li.evan.chao@gmail.com> wrote in

Or we can just undef the macros before closing the functions.

Yeah, that’s also an option - we’ve seen it adopted elsewhere.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At 2026-02-03 10:13:53, "Chao Li" <li.evan.chao@gmail.com> wrote:

On Feb 3, 2026, at 09:58, zhang_tian_chen@163.com wrote:

Hi hackers,

There is a misuse of macro when we output directory information in KillExistingWALSummaries(), pg_resetwal.c. Which should be WALSUMMARYDIR instead of ARCHSTATDIR.

Best regards,
Tianchen Zhang<v1-0001-Fix-incorrect-directory-macro-in-KillExistingWALS.patch>

Indeed a bug. Looking at the code:

```
/*
* Remove existing WAL summary files
*/
static void
KillExistingWALSummaries(void)
{
DIR *xldir;

xldir = opendir(WALSUMMARYDIR);
if (xldir == NULL)
pg_fatal("could not open directory \"%s\": %m", WALSUMMARYDIR);
...

if (errno)
pg_fatal("could not read directory \"%s\": %m", WALSUMMARYDIR);

if (closedir(xldir))
pg_fatal("could not close directory \"%s\": %m", ARCHSTATDIR); <<=== It should really be WALSUMMARYDIR
}
```

I guess closedir() is hard to fail, that’s why the problem has not been noticed earlier.

The patch is straightforward and looks correct.

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

As the discussion above, I've updated the patch to v2 that also undefines the function-scoped marco at the end of corresponding fuctions.

Best regards,
Tianchen Zhang

On Feb 3, 2026, at 14:16, Tianchen Zhang <zhang_tian_chen@163.com> wrote:

At 2026-02-03 10:13:53, "Chao Li" <li.evan.chao@gmail.com> wrote:

On Feb 3, 2026, at 09:58, zhang_tian_chen@163.com wrote:

Hi hackers,

There is a misuse of macro when we output directory information in KillExistingWALSummaries(), pg_resetwal.c. Which should be WALSUMMARYDIR instead of ARCHSTATDIR.

Best regards,
Tianchen Zhang<v1-0001-Fix-incorrect-directory-macro-in-KillExistingWALS.patch>

Indeed a bug. Looking at the code:

```
/*
* Remove existing WAL summary files
*/
static void
KillExistingWALSummaries(void)
{
DIR *xldir;

xldir = opendir(WALSUMMARYDIR);
if (xldir == NULL)
pg_fatal("could not open directory \"%s\": %m", WALSUMMARYDIR);
...

if (errno)
pg_fatal("could not read directory \"%s\": %m", WALSUMMARYDIR);

if (closedir(xldir))
pg_fatal("could not close directory \"%s\": %m", ARCHSTATDIR); <<=== It should really be WALSUMMARYDIR
}
```

I guess closedir() is hard to fail, that’s why the problem has not been noticed earlier.

The patch is straightforward and looks correct.

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

As the discussion above, I've updated the patch to v2 that also undefines the function-scoped marco at the end of corresponding fuctions.

Best regards,
Tianchen Zhang
<v2-0001-Fix-incorrect-directory-macro-in-KillExistingWALS.patch>

Thanks for updating the patch.

I applied v2 locally and the build passed. I also tried to revert to the wrong macro, now the compiler caught the mistake:
```
pg_resetwal.c:1116:52: error: use of undeclared identifier 'ARCHSTATDIR'
1116 | pg_fatal("could not close directory \"%s\": %m", ARCHSTATDIR);
| ^
1 error generated.
make[3]: *** [pg_resetwal.o] Error 1
```

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

At Tue, 3 Feb 2026 14:16:56 +0800 (CST), "Tianchen Zhang" <zhang_tian_chen@163.com> wrote in

As the discussion above, I've updated the patch to v2 that also undefines the function-scoped marco at the end of corresponding fuctions.

Thanks, looks good to me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Feb 03, 2026 at 03:49:47PM +0900, Kyotaro Horiguchi wrote:

Thanks, looks good to me.

Yep, let's fix that. Done.
--
Michael