Re: Add contrib/pg_logicalsnapinspect

On Mon, Sep 16, 2024 at 8:03 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Mon, Sep 16, 2024 at 04:02:51PM +0530, shveta malik wrote:
> > On Wed, Sep 11, 2024 at 4:21 PM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > >
> > >
> > > Yeah, good idea. Done that way in v3 attached.
> > >
> >
> > Thanks for the patch. +1 on the patch's idea. I have started
> > reviewing/testing it. It is WIP but please find few initial comments:
>
> Thanks for sharing your thoughts and for the review!
>
> >
> > src/backend/replication/logical/snapbuild.c:
> >
> > 1)
> > + fsync_fname("pg_logical/snapshots", true);
> >
> > Should we use macros PG_LOGICAL_DIR and PG_LOGICAL_SNAPSHOTS_DIR in
> > ValidateSnapshotFile(), instead of hard coding the path
> >
> >
> > 2)
> > Same as above in pg_get_logical_snapshot_meta() and
> > pg_get_logical_snapshot_info()
> >
> > + sprintf(path, "pg_logical/snapshots/%X-%X.snap",
> > + LSN_FORMAT_ARGS(lsn)); LSN_FORMAT_ARGS(lsn));
> >
>
> Doh! Yeah, agree that we should use those macros. They are coming from c39afc38cf
> which has been introduced after v1 of this patch. I thought I took care of it once
> c39afc38cf went in, but it looks like I missed it somehow. Done in v4 attached,
> Thanks!
>
> > 3)
> > +#include "replication/internal_snapbuild.h"
> >
> > Shall we name new file as 'snapbuild_internal.h' instead of
> > 'internal_snapbuild.h'. Please see other files' name under
> > './src/include/replication':
> > worker_internal.h
> > walsender_private.h
>
> Agree, that should be snapbuild_internal.h, done in v4.
>
> >
> > 4)
> > +static void ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
> > + const char *path);
> >
> > Is it required? We generally don't add declaration unless required by
> > compiler. Since definition is prior to usage, it is not needed?
> >
>
> I personally prefer to add them even if not required by the compiler. I did not
> pay attention that "We generally don't add declaration unless required by compiler"
> and (after a quick check) I did not find any reference in the coding style
> documentation [1]. That said, I don't have a strong opinion about that and so
> removed in v4. Worth to add a mention in the coding convention doc?
>

Okay. 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.

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

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.

3)
Similar change can be done here:

+ values[0] = Int32GetDatum(ondisk.magic);
+ values[1] = Int32GetDatum(ondisk.checksum);
+ values[2] = Int32GetDatum(ondisk.version);

check at the end will be: Assert(i == PG_GET_LOGICAL_SNAPSHOT_META_COLS);

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)

thanks
Shveta