Hi,
On Wed, Mar 05, 2025 at 10:42:35PM -0800, Masahiko Sawada wrote:
> On Tue, Mar 4, 2025 at 10:44 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 04, 2025 at 09:45:54PM +0000, Bertrand Drouvot wrote:
> > > Indeed, thanks for looking at it! Fixed in v4 attached. Note that the pfree()
> > > in parse_snapshot_filename() is not needed per say because the function is
> > > currently executed in a short-lived memory context. It's there for safety reason
> > > in case it's called outside those SQL apis in the future.
> >
> > After sleeping on it, PFA a simplified version.
> >
>
> Thank you for updating the patch.
>
> I think we don't need to even do palloc() for the buffer as we can use
> the char[MAXPGPATH] instead.
Sure.
> I've attached the patch to improve the
> parse_snapshot_filename() function and add some regression tests.
> Please review these changes.
Thanks for the patch!
=== 1
-parse_snapshot_filename(const char *filename)
+parse_snapshot_filename(char *filename)
Why?
=== 2
- if (sscanf(filename, "%X-%X", &hi, &lo) != 2)
+ if (sscanf(filename, "%X-%X.snap", &hi, &lo) != 2)
We could replace (sscanf(filename, "%X-%X.snap", &hi, &lo) != 2) with
(sscanf(filename, "%X-%X.foo", &hi, &lo) != 2) and the regression tests would
still pass.
So, I think it's better to remove the .snap here as it could give the "wrong"
impression that it's "useful".
The attached removes the .snap and adds a comment like:
"
* Note: We deliberately don't use "%X-%X.snap" because sscanf only counts
* converted values (%X), not literal text matches.
"
I think it makes sense to document this behavior.
=== 3
+ /*
+ * Bring back the LSN to the snapshot file format and compare
+ * it to the given name to see if the extracted LSN is sane.
+ */
+ sprintf(tmpfname, "%X-%X.snap", hi, lo);
+ if (strcmp(tmpfname, filename) != 0)
The idea was also to ensure that there are no extra characters between the LSN
values and the .snap extension: Adding this as an extra comment in the attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
