public inbox for [email protected]
help / color / mirror / Atom feedFrom: Masahiko Sawada <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: [email protected]
Subject: Re: Add contrib/pg_logicalsnapinspect
Date: Thu, 10 Oct 2024 17:38:43 -0700
Message-ID: <CAD21AoCoyNBCahi6efsjG9Lr_dGsVin1m6Y6tj4dTE6UnTAcbw@mail.gmail.com> (raw)
In-Reply-To: <ZwfSXS/[email protected]>
References: <[email protected]>
<CAD21AoBHkL-C22pgGJCA_8Jv6HjYq0c_1Y_b2Xktk7_SdUqxqA@mail.gmail.com>
<ZwOkPDCKQ/[email protected]>
<CAHut+PvtLb6g1L9C_zbufuHbJTWNq-BeseCpVgjsabG0md6_8g@mail.gmail.com>
<[email protected]>
<CAD21AoAO3zyooPQ+E2rpEi1YBc+gxZr0CUtJwxaFJ-WWimJNvA@mail.gmail.com>
<ZwY68OhKx/[email protected]>
<CAD21AoCAEXmxaDHAjMwqcpV6JVa91wBbXamQbk-rv1QnYhzGhA@mail.gmail.com>
<ZwdK1UYdLn/[email protected]>
<CAD21AoB-bxBqBEt9zbUQKTTUag1_NRPeT_5aGXC_j+z0+T3Hkw@mail.gmail.com>
<ZwfSXS/[email protected]>
On Thu, Oct 10, 2024 at 6:10 AM Bertrand Drouvot
<[email protected]> 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
> > <[email protected]> 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
view thread (36+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add contrib/pg_logicalsnapinspect
In-Reply-To: <CAD21AoCoyNBCahi6efsjG9Lr_dGsVin1m6Y6tj4dTE6UnTAcbw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox