| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | BharatDB <bharatdbpg(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 01:42:32 |
| Message-ID: | 437572B7-4F85-460F-946F-37FE0E0178E2@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> 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/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Fujii Masao | 2025-11-12 01:56:39 | Re: [PATCH] Add hints for invalid binary encoding names in encode/decode functions |
| Previous Message | Michael Paquier | 2025-11-12 01:36:54 | Re: Add tests for object size limits of injection points |