From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | shveta malik <shveta(dot)malik(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Add contrib/pg_logicalsnapinspect |
Date: | 2024-09-24 16:51:25 |
Message-ID: | ZvLuDfEUow6gw2Hr@ip-10-97-1-34.eu-west-3.compute.internal |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On Mon, Sep 23, 2024 at 12:27:27PM +1000, Peter Smith wrote:
> My review comments for v8-0001
>
> ======
> contrib/pg_logicalinspect/pg_logicalinspect.c
>
> 1.
> +/*
> + * Lookup table for SnapBuildState.
> + */
> +
> +#define SNAPBUILD_STATE_INCR 1
> +
> +static const char *const SnapBuildStateDesc[] = {
> + [SNAPBUILD_START + SNAPBUILD_STATE_INCR] = "start",
> + [SNAPBUILD_BUILDING_SNAPSHOT + SNAPBUILD_STATE_INCR] = "building",
> + [SNAPBUILD_FULL_SNAPSHOT + SNAPBUILD_STATE_INCR] = "full",
> + [SNAPBUILD_CONSISTENT + SNAPBUILD_STATE_INCR] = "consistent",
> +};
> +
> +/*
>
> nit - the SNAPBUILD_STATE_INCR made this code appear more complicated
> than it is. Please take a look at the attachment for an alternative
> implementation which includes an explanatory comment. YMMV. Feel free
> to ignore it.
Thanks for the feedback!
I like the commment, so added it in v9 attached. OTOH I think that's better
to keep SNAPBUILD_STATE_INCR as those "+1" are all linked and that would be
easy to miss the one in pg_get_logical_snapshot_info() should we change the
increment in the future.
> ======
> src/include/replication/snapbuild.h
>
> 2.
> + * Please keep SnapBuildStateDesc[] (located in the pg_logicalinspect module)
> + * updated should a change needs to be done in SnapBuildState.
>
> nit - "...should a change needs to be done" -- the word "needs" is
> incorrect here.
>
> How about:
> "...if a change needs to be made to SnapBuildState."
Thanks, used this one in v9.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachment | Content-Type | Size |
---|---|---|
v9-0001-Add-contrib-pg_logicalinspect.patch | text/x-diff | 45.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Bertrand Drouvot | 2024-09-24 16:53:26 | Re: Add contrib/pg_logicalsnapinspect |
Previous Message | Tom Lane | 2024-09-24 16:27:59 | Re: pgsql: Improve default and empty privilege outputs in psql. |