Обсуждение: [PATCH] Fix error message in RemoveWalSummaryIfOlderThan to indicate file removal failure

Hi all,

I noticed a small issue. In `RemoveWalSummaryIfOlderThan`,an `unlink()` failure incorrectly logs "could not stat file",
likelya copy-paste error.
 
I updated it to "could not remove file". No functional changes.

```
diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 21164faac7e..4ee510092f9 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -251,7 +251,7 @@ RemoveWalSummaryIfOlderThan(WalSummaryFile *ws, time_t cutoff_time)
     if (unlink(path) != 0)
         ereport(ERROR,
                 (errcode_for_file_access(),
-                 errmsg("could not stat file \"%s\": %m", path)));
+                 errmsg("could not remove file \"%s\": %m", path)));
     ereport(DEBUG2,
             (errmsg_internal("removing file \"%s\"", path)));
 }
```

--
regards,
Man Zeng

On Sun, Feb 1, 2026 at 10:43 PM zengman <zengman@halodbtech.com> wrote:
>
> Hi all,
>
> I noticed a small issue. In `RemoveWalSummaryIfOlderThan`,an `unlink()` failure incorrectly logs "could not stat
file",likely a copy-paste error. 
> I updated it to "could not remove file". No functional changes.
>
> ```
> diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
> index 21164faac7e..4ee510092f9 100644
> --- a/src/backend/backup/walsummary.c
> +++ b/src/backend/backup/walsummary.c
> @@ -251,7 +251,7 @@ RemoveWalSummaryIfOlderThan(WalSummaryFile *ws, time_t cutoff_time)
>         if (unlink(path) != 0)
>                 ereport(ERROR,
>                                 (errcode_for_file_access(),
> -                                errmsg("could not stat file \"%s\": %m", path)));
> +                                errmsg("could not remove file \"%s\": %m", path)));
>         ereport(DEBUG2,
>                         (errmsg_internal("removing file \"%s\"", path)));
>  }
> ```
>
> --
> regards,
> Man Zeng

Agreed, that was likely due to a copy-paste oversight.

--
Regards
Junwang Zhao

On Sun, Feb 01, 2026 at 10:55:53PM +0800, Junwang Zhao wrote:
> Agreed, that was likely due to a copy-paste oversight.

Likely so.  This one should be backpatched, as the error generated
could be confusing if faced.  That's very unlikely so, still..  I'll
look at other places in the tree for similar inconsistencies, while on
it.
--
Michael

> Likely so.  This one should be backpatched, as the error generated
> could be confusing if faced.  That's very unlikely so, still..  I'll
> look at other places in the tree for similar inconsistencies, while on
> it.

Hi Michael,

Thank you for helping to address this! I’ve gone back and reviewed the code, and noticed `OpenWalSummaryFile`:
```
File
OpenWalSummaryFile(WalSummaryFile *ws, bool missing_ok)
{
    char        path[MAXPGPATH];
    File        file;

    snprintf(path, MAXPGPATH,
             XLOGDIR "/summaries/%08X%08X%08X%08X%08X.summary",
             ws->tli,
             LSN_FORMAT_ARGS(ws->start_lsn),
             LSN_FORMAT_ARGS(ws->end_lsn));

    file = PathNameOpenFile(path, O_RDONLY);
    if (file < 0 && (errno != EEXIST || !missing_ok))
        ereport(ERROR,
                (errcode_for_file_access(),
                 errmsg("could not open file \"%s\": %m", path)));

    return file;
}
```
I’m thinking of changing `errno != EEXIST` to `errno != ENOENT` here — would you think this adjustment is appropriate?

--
Regards,
Man Zeng

On Mon, Feb 2, 2026 at 11:02 AM zengman <zengman@halodbtech.com> wrote:
>
> > Likely so.  This one should be backpatched, as the error generated
> > could be confusing if faced.  That's very unlikely so, still..  I'll
> > look at other places in the tree for similar inconsistencies, while on
> > it.
>
> Hi Michael,
>
> Thank you for helping to address this! I’ve gone back and reviewed the code, and noticed `OpenWalSummaryFile`:
> ```
> File
> OpenWalSummaryFile(WalSummaryFile *ws, bool missing_ok)
> {
>         char            path[MAXPGPATH];
>         File            file;
>
>         snprintf(path, MAXPGPATH,
>                          XLOGDIR "/summaries/%08X%08X%08X%08X%08X.summary",
>                          ws->tli,
>                          LSN_FORMAT_ARGS(ws->start_lsn),
>                          LSN_FORMAT_ARGS(ws->end_lsn));
>
>         file = PathNameOpenFile(path, O_RDONLY);
>         if (file < 0 && (errno != EEXIST || !missing_ok))
>                 ereport(ERROR,
>                                 (errcode_for_file_access(),
>                                  errmsg("could not open file \"%s\": %m", path)));
>
>         return file;
> }
> ```
> I’m thinking of changing `errno != EEXIST` to `errno != ENOENT` here — would you think this adjustment is
appropriate?

+1 for this change, per the function comment.

```
As an exception, if  missing_ok = true and the trouble is specifically
that the file does not exist, it will not throw an error and will
return a value less than 0.
```

>
> --
> Regards,
> Man Zeng



--
Regards
Junwang Zhao

On Mon, Feb 02, 2026 at 11:02:39AM +0800, zengman wrote:
> Thank you for helping to address this! I’ve gone back and reviewed
> the code, and noticed `OpenWalSummaryFile`:
> ```
> File
> OpenWalSummaryFile(WalSummaryFile *ws, bool missing_ok)
> {
>     char        path[MAXPGPATH];
>     File        file;
>
>     snprintf(path, MAXPGPATH,
>              XLOGDIR "/summaries/%08X%08X%08X%08X%08X.summary",
>              ws->tli,
>              LSN_FORMAT_ARGS(ws->start_lsn),
>              LSN_FORMAT_ARGS(ws->end_lsn));
>
>     file = PathNameOpenFile(path, O_RDONLY);
>     if (file < 0 && (errno != EEXIST || !missing_ok))
>         ereport(ERROR,
>                 (errcode_for_file_access(),
>                  errmsg("could not open file \"%s\": %m", path)));
>
>     return file;
> }
> ```
> I’m thinking of changing `errno != EEXIST` to `errno != ENOENT` here
> — would you think this adjustment is appropriate?

Funny timing.  I've spotted the exact same thing while double-checking
the file two hours ago, where EEXIST should be replaced by ENOENT.  I
was going to spawn a new thread about that with Robert in CC.
--
Michael

> Funny timing.  I've spotted the exact same thing while double-checking
> the file two hours ago, where EEXIST should be replaced by ENOENT.  I
> was going to spawn a new thread about that with Robert in CC.

How funny! I'll keep an eye on the new thread. 

https://www.postgresql.org/message-id/aYAf8qDHbpBZ3Rml%40paquier.xyz

Many thanks to you both.

--
Regards,
Man Zeng