Re: Persist injection points across server restarts - Mailing list pgsql-hackers

From Rahila Syed
Subject Re: Persist injection points across server restarts
Date
Msg-id CAH2L28t4jGQ+iEbg=4ZN-akw6hOtgsjNEPYgzLzypHV-Hkij2A@mail.gmail.com
Whole thread Raw
In response to Persist injection points across server restarts  (Michael Paquier <michael@paquier.xyz>)
List 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

pgsql-hackers by date:

Previous
From: "Zhijie Hou (Fujitsu)"
Date:
Subject: RE: Conflict detection for update_deleted in logical replication
Next
From: Feike Steenbergen
Date:
Subject: Re: pg18: Virtual generated columns are not (yet) safe when superuser selects from them