Re: BUG #18828: Crash when pg_get_logical_snapshot_meta() passed empty string

Hi,

On Tue, Mar 04, 2025 at 12:11:07PM -0800, Masahiko Sawada wrote:
> On Tue, Mar 4, 2025 at 2:45 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 04, 2025 at 07:38:14AM +0000, Bertrand Drouvot wrote:
> > > PFA 0001 a new version to also parse the ".snap".
> >
> > PFA v3 (a slightly different version) to "correctly" use the new report_error
> > introduced in v2.
> 
> Thank you for the patch! I agree to have some regression tests for that.
> 
> Looking at the 0001 patch, it seems that the pg_logicalinspect
> functions still accepts invalid filenames as follows:
> 
> postgres(1:2713976)=# select
> pg_get_logical_snapshot_info('0-40796E18.foo.snap');
> ERROR:  could not open file "pg_logical/snapshots/0-40796E18.snap": No
> such file or directory

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.

> Regarding the 0002 patch, TBH I'm confused about what the original
> comment intended to mean. I thought that it means that we don't need
> to use a critical section for fsync_fname() calls to promote the fsync
> error.

Yeah I think the same.

> If that's right, it would be better to update the comment
> instead of removing it.

hmm I'm not sure what to say then. It was saying that there is not need to promote
to PANIC because it won't be usable anyway (if not fsynced).

Since 9ccdd7f66e3 we do promote to PANIC so I thought the most simple to avoid
any extra confusion is to remove the comment (since it's wrong since 9ccdd7f66e3).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com