Re: Add contrib/pg_logicalsnapinspect

On Tue, Sep 17, 2024 at 12:44 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, Sep 17, 2024 at 10:18:35AM +0530, shveta malik wrote:
> > Thanks for addressing the comments. I have not started reviewing v4
> > yet, but here are few more comments on v3:
> >
> > 1)
> > +#include "port/pg_crc32c.h"
> >
> > It is not needed in pg_logicalinspect.c as it is already included in
> > internal_snapbuild.h
>
> Yeap, forgot to remove that one when creating the new "internal".h file, done
> in v5 attached, thanks!
>
> >
> > 2)
> > + values[0] = Int16GetDatum(ondisk.builder.state);
> > ........
> > + values[8] = LSNGetDatum(ondisk.builder.last_serialized_snapshot);
> > + values[9] = TransactionIdGetDatum(ondisk.builder.next_phase_at);
> > + values[10] = Int64GetDatum(ondisk.builder.committed.xcnt);
> >
> > We can have values[i++] in all the places and later we can check :
> > Assert(i == PG_GET_LOGICAL_SNAPSHOT_INFO_COLS);
> > Then we need not to keep track of number even in later part of code,
> > as it goes till 14.
>
> Right, let's do it that way (as it is done in pg_walinspect for example).
>
> > 4)
> > Most of the output columns in pg_get_logical_snapshot_info() look
> > self-explanatory except 'state'. Should we have meaningful 'text' here
> > corresponding to SnapBuildState? Similar to what we do for
> > 'invalidation_reason' in pg_replication_slots. (SlotInvalidationCauses
> > for ReplicationSlotInvalidationCause)
>
> Yeah we could. I was not sure about that (and that was my first remark in [1])
> , as the module is mainly for debugging purpose, I was thinking that the one
> using it could refer to "snapbuild.h". Let's see what others think.
>

okay, makes sense. lets wait what others have to say.

Thanks for the patch. Few trivial things:

1)
May be we shall change 'INTERNAL_SNAPBUILD_H' in snapbuild_internal.h
to 'SNAPBUILD_INTERNAL_H'?

2)
ValidateSnapshotFile()

It is not only validating, but loading the content as well. So may be
we can rename to ValidateAndRestoreSnapshotFile?

3) sgml:
a)
+ The pg_logicalinspect functions are called using an LSN argument
that can be extracted from the output name of the
pg_ls_logicalsnapdir() function.

Is it possible to give link to pg_ls_logicalsnapdir function here?

b)
+ Gets logical snapshot metadata about a snapshot file that is located
in the pg_logical/snapshots directory.

located in server's pg_logical/snapshots directory
(i.e. use server keyword, similar to how pg_ls_logicalsnapdir ,
pg_ls_logicalmapdir explains it)

thanks
Shveta