Re: Add contrib/pg_logicalsnapinspect

On Thu, Oct 10, 2024 at 6:10 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Thu, Oct 10, 2024 at 12:05:10AM -0700, Masahiko Sawada wrote:
> > On Wed, Oct 9, 2024 at 8:32 PM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > > So I think that having construct_array_builtin()/deconstruct_array_builtin()
> > > taking care of XIDOID is the way to go. If that makes sense to you then 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 deduced
> from:
>
> postgres=# select typbyval, typalign from pg_type where typname = 'xid';
> 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) != TYPEFUNC_COMPOSITE)
+ 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++] = 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 otherwise.
+ * 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