On Sun, Oct 13, 2024 at 11:23 PM Bertrand Drouvot
<bertranddrouvot...@gmail.com> wrote:
>
> Hi,
>
> On Mon, Oct 14, 2024 at 09:57:22AM +1100, Peter Smith wrote:
> > Here are some minor review comments for v15-0002.
> >
> > ======
> > contrib/pg_logicalinspect/pg_logicalinspect.c
> >
> > 1.
> > +pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
> > +{
> > +#define PG_GET_LOGICAL_SNAPSHOT_META_COLS 3
> > + SnapBuildOnDisk ondisk;
> > + HeapTuple tuple;
> > + Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
> > + bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
> > + TupleDesc tupdesc;
> > + char path[MAXPGPATH];
> > + int i = 0;
> > + text *filename_t = PG_GETARG_TEXT_PP(0);
> > +
> > + sprintf(path, "%s/%s",
> > + PG_LOGICAL_SNAPSHOTS_DIR,
> > + text_to_cstring(filename_t));
> > +
> > + /* Build a tuple descriptor for our result type */
> > + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> > + elog(ERROR, "return type must be a row type");
> > +
> > + /* Validate and restore the snapshot to 'ondisk' */
> > + SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
> >
> > The sprintf should be deferred. Could you do it after the ERROR check?
>
> I think that makes sense, done in v16 attached.
>
> > ======
> > src/backend/replication/logical/snapbuild.c
> >
> > 3.
> > /*
> > - * Restore a snapshot into 'builder' if previously one has been stored at
> > the
> > - * location indicated by 'lsn'. Returns true if successful, false
> > otherwise.
> > + * Restore the logical snapshot file contents to 'ondisk'.
> > + *
> > + * If 'missing_ok' is true, will not throw an error if the file is not
> > found.
> > + * 'context' is the memory context where the catalog modifying/committed
> > xid
> > + * will live.
> > */
> > -static bool
> > -SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
> > +bool
> > +SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
> > + MemoryContext context, bool missing_ok)
> >
> > nit - I think it's better to describe parameters in the same order
> > that they are declared.
>
> Done in v16.
>
> > Also, include a 'path' description, so it is
> > not the only one omitted.
>
> I don't think that's worth it as self explanatory IMHO.
Thank you for updating the patches!
I fixed a compiler warning by -Wtypedef-redefinition related to the
declaration of SnapBuild struct, then pushed both patches.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
