Re: Add contrib/pg_logicalsnapinspect

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.

[1]: https://www.postgresql.org/message-id/ZscuZ92uGh3wm4tW%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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