Re: Persist injection points across server restarts
Rahila Syed <rahilasyed90@gmail.com>
From: Rahila Syed <rahilasyed90@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>, "Maksim.Melnikov" <m.melnikov@postgrespro.ru>
Date: 2025-05-26T07:47:46Z
Lists: pgsql-hackers
Hi, > This is v19 work, so I am adding that to the next commit fest. > > Rebased, to fix a conflict I've introduced with a different commit. > > Thank you for the rebased patches. I reviewed the code and have a few comments for your consideration. It appears that Github CI is reporting failures with injection_points/002_data_persist failing across all OSes. Following are comments on v2-0002-injection_points-Add-function-to-flush-injection-.patch 1. /* + * Rename file into place, so we atomically replace any old one. + */ + (void) durable_rename(INJ_DUMP_FILE_TMP, INJ_DUMP_FILE, LOG); I wonder if it would be better to include a check for the return value of `durable_rename` to manage potential failure scenarios. 2. + + file = AllocateFile(INJ_DUMP_FILE ".tmp", PG_BINARY_W); Could we perhaps add a brief comment clarifying the rationale behind the use of a temporary file in this context? 3. + if (fread(buf, 1, len + 1, file) != len + 1) + goto error; + library = pstrdup(buf); It seems that `buf` should be null-terminated before being passed to `pstrdup(buf)`. Otherwise, `strlen` might not accurately determine the length. This seems to be consistent with the handling of `fread` calls in `snapmgr.c`. Thank you, Rahila Syed
Commits
-
Add InjectionPointList() to retrieve list of injection points
- 7b2eb72b1b8c 19 (unreleased) landed