Re: Add contrib/pg_logicalsnapinspect

Hi,

On Wed, Sep 18, 2024 at 07:52:51PM +1000, Peter Smith wrote:
> HI, here are some mostly minor review comments for the patch v5-0001.
> 

Thanks for the review!

> ======
> Commit message
> 
> 1.
> Do you think you should also name the new functions here?

Not sure about this one. It has not been done in 2258e76f90 for example.

> 
> ======
> contrib/pg_logicalinspect/pg_logicalinspect.c
> 
> 2.
> Regarding the question about static function declarations:
> 
> Shveta wrote: I was somehow under the impression that this is the way
> in the postgres i.e. not add redundant declarations.  Will be good to
> know what others think on this.
> 
> FWIW, my understanding is the convention is just to be consistent with
> whatever the module currently does. If it declares static functions,
> then declare them all (redundant or not). If it doesn't declare static
> functions, then don't add one. But, in the current case, since this is
> a new module, I guess it is entirely up to you whatever you want to
> do.
>

Thanks for the feedback and sharing your thoughts. I don't have a strong opinion
on this (though I tend to write the declaration(s)). I see it as a Nit, so let
just keep it as done in v6.
 
> ~~~
> 
> 3.
> +/*
> + * NOTE: For any code change or issue fix here, it is highly recommended to
> + * give a thought about doing the same in SnapBuildRestore() as well.
> + */
> +
> 
> nit - I think this NOTE should be part of this module's header
> comment. (e.g. like the tablesync.c NOTES)
>

Not sure about this one. I took pg_walinspect.c as an example. And we may want
to add more functionalities in the future that could have nothing to do with
SnapBuildRestore(). I think that I prefer where it is located currently (near the
code that "looks like" SnapBuildRestore()).
 
> ~~~
> 
> ValidateSnapshotFile:
> 
> 4.
> +ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk, const char *path)
> +{
> + int fd;
> + Size sz;
> 
> nit - The 'sz' is overwritten a few times. I thnk declaring it at each
> scope where used would be tidier.

I see what you mean. I think it's a matter of taste and I generally also prefer
to do it the way you propose. That said, it's mainly inspired from SnapBuildRestore(),
so I think it's better to try to differ from it the less that we can.

> 
> ~~~
> 
> 5.
> + fsync_fname(path, false);
> + fsync_fname(PG_LOGICAL_SNAPSHOTS_DIR, true);
> +
> +
> 
> nit - remove some excessive blank lines

Agree. While it's the same as in SnapBuildRestore(), I'm ok to do this particular
change, done in v7 attached.

> 
> ~~~
> 
> 6.
> + /* read statically sized portion of snapshot */
> + SnapBuildRestoreContents(fd, (char *) ondisk,
> SnapBuildOnDiskConstantSize, path);
> 
> Should that say "fixed size portion"?
>

Maybe, but same remark as for 4. (though that's only a comment).
 
> ~~~
> 
> pg_get_logical_snapshot_info:
> 
> 7.
> + if (ondisk.builder.committed.xcnt > 0)
> + {
> + Datum    *arrayelems;
> + int narrayelems;
> +
> + arrayelems = (Datum *) palloc(ondisk.builder.committed.xcnt * sizeof(Datum));
> + narrayelems = 0;
> +
> + for (narrayelems = 0; narrayelems < ondisk.builder.committed.xcnt;
> narrayelems++)
> + arrayelems[narrayelems] = Int64GetDatum((int64)
> ondisk.builder.committed.xip[narrayelems]);
> 
> nit - Why the double assignment of narrayelems = 0?

Probably fat fingers when writting this part.

> assign at the declaration and then remove both others.

yeah, done.
 
> ======
> doc/src/sgml/pglogicalinspect.sgml
> 
> 9.
> + <para>
> +  The <filename>pg_logicalinspect</filename> module provides SQL functions
> +  that allow you to inspect the contents of logical decoding components. It
> +  allows to inspect serialized logical snapshots of a running
> +  <productname>PostgreSQL</productname> database cluster, which is useful
> +  for debugging or educational purposes.
> + </para>
> 
> nit - /It allows to inspect/It allows the inspection of/

Done.

> 
> ~~~
> 
> 10.
> +      example:
> 
> nit - /example:/For example:/ (this is in a couple of places)

Done.

> 
> ======
> src/include/replication/snapbuild_internal.h
> 
> 11.
> +#ifndef INTERNAL_SNAPBUILD_H
> +#define INTERNAL_SNAPBUILD_H
> 
> Shouldn't these be SNAPBUILD_INTERNAL_H to match the filename?

Yeah, was already mentioned by Shveta up-thread and fixed in v6.

> ~~~
> 
> 12.
> The contents of the snapbuild.c that got moved into
> snapbuild_internal.h also got shuffled around a bit.
> 
> e.g. originally the typedef struct SnapBuildOnDisk:
> 
> +/*
> + * We store current state of struct SnapBuild on disk in the following manner:
> + *
> + * struct SnapBuildOnDisk;
> + * TransactionId * committed.xcnt; (*not xcnt_space*)
> + * TransactionId * catchange.xcnt;
> + *
> + */
> +typedef struct SnapBuildOnDisk
> 
> was directly beneath the comment:
> -/* -----------------------------------
> - * Snapshot serialization support
> - * -----------------------------------
> - */
> -
>

Moving it to the same place in v7.
 
> The macros were also defined immediately after the SnapBuildOnDisk
> fields they referred to.
> 

Moving them to the same place in v7.

> Please also see the attachment, which implements some of those nits
> mentioned above.

While I did not implement all the nits you mentioned, I really appreciate that
you added this attachment, thanks! That's a nice idea and I will try to do the
same for my reviews..

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com