MIME-Version: 1.0
References: <ZwY68OhKx/5jWeCj@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoCAEXmxaDHAjMwqcpV6JVa91wBbXamQbk-rv1QnYhzGhA@mail.gmail.com>
 <ZwdK1UYdLn/zFMHy@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoB-bxBqBEt9zbUQKTTUag1_NRPeT_5aGXC_j+z0+T3Hkw@mail.gmail.com>
 <ZwfSXS/3v73388jh@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoCoyNBCahi6efsjG9Lr_dGsVin1m6Y6tj4dTE6UnTAcbw@mail.gmail.com>
 <Zwkk9Z6rzNxArr1p@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoB31apHCzZ-45rDtB56qQm+TBwJnB48JjBvuqR5xviAEQ@mail.gmail.com>
 <CAD21AoD+opO0SaDSFvz+n+wAxc-kUrpF5Wtxbw80Bqpay+rkgw@mail.gmail.com>
 <CAHut+PsYdTW4+U+fioh9_yO8iT6RnY=PquzfCZu7zRU6Qg6W7g@mail.gmail.com> <Zwy4ys76aPcoVE/I@ip-10-97-1-34.eu-west-3.compute.internal>
In-Reply-To: <Zwy4ys76aPcoVE/I@ip-10-97-1-34.eu-west-3.compute.internal>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 14 Oct 2024 18:08:10 -0700
Message-ID: <CAD21AoDuO87MFrPJFwtmRi=cFhDGac0zq7sq5pdEhVi7zUvbEg@mail.gmail.com>
Subject: Re: Add contrib/pg_logicalsnapinspect
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, 
	shveta malik <shveta.malik@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAD21AoDuO87MFrPJFwtmRi%3DcFhDGac0zq7sq5pdEhVi7zUvbEg%40mail.gmail.com>
Precedence: bulk

On Sun, Oct 13, 2024 at 11:23=E2=80=AFPM Bertrand Drouvot
<bertranddrouvot.pg@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.
> >
> > =3D=3D=3D=3D=3D=3D
> > 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] =3D {0};
> > + bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] =3D {0};
> > + TupleDesc tupdesc;
> > + char path[MAXPGPATH];
> > + int i =3D 0;
> > + text    *filename_t =3D 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) !=3D TYPEFUNC_COMPOS=
ITE)
> > + 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.
>
> > =3D=3D=3D=3D=3D=3D
> > 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 othe=
rwise.
> > + * 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/committ=
ed 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,

--=20
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com