Re: Non-blocking archiver process

From: BharatDB <bharatdbpg(at)gmail(dot)com>
To: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Artem Gavrilov <artem(dot)gavrilov(at)percona(dot)com>, Patrick Stählin <me(at)packi(dot)ch>, Noah Misch <noah(at)leadboat(dot)com>, Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Non-blocking archiver process
Date: 2025-11-12 11:58:37
Message-ID: CAAh00EQOhfzgzOAECAWJRgyx6wyPiUnJfb3jQJdTaWeOip7DxA@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Evan and PostgreSQL community,

Thank you for the feedback and for reporting the build failure.

I have prepared v2 of the patch, which fixes the issues you mentioned:

- Replaced the incorrect include `#include "latch.h"` with `#include
"storage/latch.h"`, fixing the macOS build failure.
- Wrapped Windows-specific code (`win32cmd`, `pfree(win32cmd)`, etc.)
inside `#ifdef WIN32`.
- Used `OpenPipeStream()` and `fileno(archiveFile)` for safe non-blocking
archive command execution.
- Switched to `palloc()`/`pfree()` for memory safety in backend context.
- Read and logged command stdout to avoid hangs.
- Cleaned up redundant `return;` and unreachable code.
- Ran `pgindent` for proper PostgreSQL code style formatting.

Verification
- Built successfully on Linux with:
`make distclean && ./configure --with-zlib && make -j$(nproc)`
- Confirmed clean compile of `src/backend/archive/shell_archive.c` with no
warnings.
- Verified zlib linked (`-lz`) and archiver module builds successfully.

This v2 should now build cleanly on both macOS and Linux and follows
PostgreSQL’s coding conventions.

Attached is the updated patch:
`0001-v2-shell_archive-refactor-fix-build-and-format.patch`

Best regards,
Lakshmi
<bharatdbpg(at)gmail(dot)com>

On Wed, Nov 12, 2025 at 7:13 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:

>
>
> > On Nov 10, 2025, at 18:41, BharatDB <bharatdbpg(at)gmail(dot)com> wrote:
> >
> > Dear PostgreSQL Community,
> > I found 8 Bugs in `shell_archive.c`
> >
> > While studying how `archive_command` works, I discovered **8 real
> issues** that affect reliability, performance, and safety:
> > | # | Bug | Impact | |---|-----|--------| | 1 | Infinite loop on command
> failure | Archiver hangs forever | | 2 | Unreachable code after `return` |
> Dead logic | | 3 | Discarded `stdout` from archive command | `DROP
> DATABASE` hangs for full command duration | | 4 | Aggressive polling with
> `Sleep(100)` | CPU waste (already fixed in core) | | 5 | `malloc`/`free` in
> backend | **Memory corruption risk** | | 6 | Poor variable names (`dwRc`,
> `bytesRead`) | Hard to read | | 7 | Manual `popen` / `CreateProcess` |
> Missed PG infrastructure | | 8 | Redundant `return;` in `void` function |
> Style issue |
> > I refactored `src/backend/archive/shell_archive.c` with
> **PostgreSQL-style fixes**:
> > - Replaced `popen()` and `CreateProcess()` → `OpenPipeStream()` - Used
> `fileno(archiveFile)` → `archiveFd` correctly - Switched to `palloc()` /
> `pfree()` for memory safety - Renamed variables: `dwRc` → `exit_code`,
> `bytesRead` → `nread`, etc. - Read `stdout` to prevent `DROP DATABASE`
> hangs - Used `WaitLatchOrSocket()` for interruptible waiting - Removed
> redundant `return;` and dead code This is my contribution to improve the
> PostgreSQL archiver process. I have attached a patch implementing the
> discussed fixes — please review and share any suggestions or feedback.
> Thanks !
> > Lakshmi
> >
>
> Thanks for the patch, but I just cannot pass build in my end:
> ```
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla
> -Werror=unguarded-availability-new -Wendif-labels
> -Wmissing-format-attribute -Wcast-function-type -Wformat-security
> -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv
> -fexcess-precision=standard -Wno-unused-command-line-argument
> -Wno-compound-token-split-by-macro -Wno-format-truncation
> -Wno-cast-function-type-strict -g -O2 -I../../../src/include -isysroot
> /Library/Developer/CommandLineTools/SDKs/MacOSX15.5.sdk
> -I/opt/homebrew/Cellar/icu4c(at)77/77.1/include -c -o shell_archive.o
> shell_archive.c -MMD -MP -MF .deps/shell_archive.Po
> shell_archive.c:19:10: fatal error: 'latch.h' file not found
> 19 | #include "latch.h" /* For WaitLatchOrSocket */
> | ^~~~~~~~~
> 1 error generated.
> make[3]: *** [shell_archive.o] Error 1
> make[2]: *** [archive-recursive] Error 2
> make[1]: *** [all-backend-recurse] Error 2
> make: *** [all-src-recurse] Error 2
> ```
>
> Also, as a general comment, you need do pgident on the file you changed.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
>

Attachment Content-Type Size
0001-v2-shell_archive-refactor-fix-build-and-format.patch text/x-patch 8.3 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Akshay Joshi 2025-11-12 12:04:44 [PATCH] Add pg_get_database_ddl() function to reconstruct CREATE DATABASE statement
Previous Message shveta malik 2025-11-12 11:42:20 Re: POC: enable logical decoding when wal_level = 'replica' without a server restart