MIME-Version: 1.0
References: <ZvRIZctV1M4PPmwT@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoBHkL-C22pgGJCA_8Jv6HjYq0c_1Y_b2Xktk7_SdUqxqA@mail.gmail.com>
 <ZwOkPDCKQ/gDNuZL@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAHut+PvtLb6g1L9C_zbufuHbJTWNq-BeseCpVgjsabG0md6_8g@mail.gmail.com>
 <ZwVc8SZWvTlzbE6x@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoAO3zyooPQ+E2rpEi1YBc+gxZr0CUtJwxaFJ-WWimJNvA@mail.gmail.com>
 <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>
In-Reply-To: <ZwfSXS/3v73388jh@ip-10-97-1-34.eu-west-3.compute.internal>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 10 Oct 2024 17:38:43 -0700
Message-ID: <CAD21AoCoyNBCahi6efsjG9Lr_dGsVin1m6Y6tj4dTE6UnTAcbw@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/CAD21AoCoyNBCahi6efsjG9Lr_dGsVin1m6Y6tj4dTE6UnTAcbw%40mail.gmail.com>
Precedence: bulk

On Thu, Oct 10, 2024 at 6:10=E2=80=AFAM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Thu, Oct 10, 2024 at 12:05:10AM -0700, Masahiko Sawada wrote:
> > On Wed, Oct 9, 2024 at 8:32=E2=80=AFPM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > > So I think that having construct_array_builtin()/deconstruct_array_bu=
iltin()
> > > taking care of XIDOID is the way to go. If that makes sense to you th=
en I'll
> > > submit a dedicated patch for it, thoughts?
> >
> > Your explanation makes sense to me.
>
> Thanks for sharing your thoughts.
>
> > I think it can be included in the main pg_logicalinspect patch as this =
change
> > is a part of it.
>
> Okay, let's keep the discussion here. Please find attached v13 that takes=
 care
> of your previous remarks and Peter's one ([1]).
>
> FYI, v13 is splitted into 2 sub-patches (0001 for the discussion related =
to
> XIDOID and construct_array_builtin() and 0002 for the module itself).

Thank you for updating the patch!

>
> FWIW, the elmbyval and elmalign values that are added in 0001 have been d=
educed
>  from:
>
> postgres=3D# select typbyval, typalign from pg_type where typname =3D 'xi=
d';
>  typbyval | typalign
> ----------+----------
>  t        | i
> (1 row)

+1

The patches mostly look good to me. Here are some minor comments:

+       sprintf(path, "%s/%s",
+                       PG_LOGICAL_SNAPSHOTS_DIR,
+                       text_to_cstring(filename_t));
+
+       /* Validate and restore the snapshot to 'ondisk' */
+       ValidateAndRestoreSnapshotFile(&ondisk, path,
CurrentMemoryContext, false);
+
+       /* Build a tuple descriptor for our result type */
+       if (get_call_result_type(fcinfo, NULL, &tupdesc) !=3D TYPEFUNC_COMP=
OSITE)
+               elog(ERROR, "return type must be a row type");
+
I think it would be better to check the result type before reading the
snapshot file.

---
+       values[i++] =3D Int64GetDatum((int64) ondisk.checksum);

Why is only checksum casted to int64? With that, it can show a
checksum value as a non-netagive integer but is it really necessary?
For instance, page_header() function in pageinspect shows a page
checksum as smallint.

---@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/

output_iso and tmp_check_iso should be added.

---
 /*
- * Restore a snapshot into 'builder' if previously one has been stored at =
the
- * location indicated by 'lsn'. Returns true if successful, false otherwis=
e.
+ * Validate the logical snapshot file and read its contents to 'ondisk'.
  */
-static bool
-SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
+bool
+ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk, const char *path,
+                              MemoryContext context, bool missing_ok)

I think it would be better to add some descriptions of the function
arguments, particularly context and missing_ok.

The names of all other functions in snapbuild.c have the "SnapBuild"
prefix. I think it's better to follow it. How about renaming it to
SnapBuildReadSnapshot(), SnapBuildRestoreSnapshot(), or something
along those lines?

Regards,


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