On Tue, Sep 17, 2024 at 12:44 PM Bertrand Drouvot <bertranddrouvot...@gmail.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