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

Hi,

On Mon, Mar 03, 2025 at 12:42:01PM -0800, Masahiko Sawada wrote:
> On Mon, Mar 3, 2025 at 12:50 AM Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >
> > Now I wonder if pg_get_logical_snapshot_meta() (and pg_get_logical_snapshot_info()
> > which suffers for the same issues reported above) should take a lsn as input
> > parameter. That's how it has been proposed initially and that would simplify
> > things. Thoughts?
>
> Yeah, that's one solution. But it would make pg_logicalinspect
> functions hard to work with the pg_ls_loigcalsnapdir(). Users would
> need to parse the filename by themselve and pass the LSN to them. I've
> attached a draft patch for a different approach where we extract LSN
> from the given file name in pg_logicalinspect functions. Thoughts?

Yeah I agree that your proposal is more user friendly. Thanks for the patch,
you beat me on it! (I was waiting that we agree on the approach before working
on it).

Looking at your patch, some comments:

=== 1

+ if (sscanf(filename, "%X-%X.snap", &hi, &lo) != 2)
+ ereport(ERROR,

sscanf would not return an error if the file being used is say "0-40796E18.foo":

postgres=# SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
pg_get_logical_snapshot_info
-------------------------------------------------------------------------
(consistent,751,751,0/40796AF8,0/40796AF8,0,f,f,0/0,0,0,,2,"{751,752}")

I think that as long as it finds the &hi and &lo then it does not return an
error.

PFA 0001 a new version to also parse the ".snap".

=== 2

+ errmsg("invalid serialized snapshot file name \"%s\"", filename));

In the extension doc we mention "a snapshot file" so I think it makes sense to
remove "serialized" from the error message: also done in 0001 attached.

=== 3

+ * Extract LSN from the given serialized snapshot

s/Extract LSN/Extract the LSN/? (done in 0001 attached).

=== 4

Also adding some regression tests in 0001 attached to validate the imputs
and the permissions.

> > Yeah. That's the fsync_fname() call in SnapBuildRestoreSnapshot() that promotes
> > to PANIC due to (data_sync_elevel(ERROR)). This behavior has been introduced in
> > 9ccdd7f66e3 (that made the comment in SnapBuildRestore() (at that time) inaccurate).
> >
> > I'm not sure we should change the logic here, maybe just update the comment?
>
> I think that we don't need to change the logic. It might be a good
> idea to clarify the comment, though.

Agree. I think that we can just get rid of it: done in 0002 attached.

Regards,

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