Horton

Re: Add contrib/pg_logicalsnapinspect

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

View thread
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-09-16T14:33:20Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix indentation issue

  2. pg_logicalinspect: Stabilize isolation tests.

  3. Add contrib/pg_logicalinspect.

  4. Move SnapBuild and SnapBuildOnDisk structs to snapshot_internal.h.

Attachments

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.pg@gmail.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?


[1]: https://www.postgresql.org/docs/current/source.html

Regards,

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