Re: Add contrib/pg_logicalsnapinspect - Mailing list pgsql-hackers
From | Masahiko Sawada |
---|---|
Subject | Re: Add contrib/pg_logicalsnapinspect |
Date | |
Msg-id | CAD21AoCoyNBCahi6efsjG9Lr_dGsVin1m6Y6tj4dTE6UnTAcbw@mail.gmail.com Whole thread Raw |
In response to | Re: Add contrib/pg_logicalsnapinspect (Masahiko Sawada <sawada.mshk@gmail.com>) |
List | pgsql-hackers |
On Thu, Oct 10, 2024 at 6:10 AM 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 PM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.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
pgsql-hackers by date: