public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bertrand Drouvot <[email protected]>
To: Peter Smith <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: [email protected]
Subject: Re: Add contrib/pg_logicalsnapinspect
Date: Fri, 11 Oct 2024 13:17:58 +0000
Message-ID: <Zwklhj/[email protected]> (raw)
In-Reply-To: <CAHut+PvRuPxs8103CSO_n_qu0BYJjJAin8yML3Go+Ur4a8tyWg@mail.gmail.com>
References: <ZwOkPDCKQ/[email protected]>
<CAHut+PvtLb6g1L9C_zbufuHbJTWNq-BeseCpVgjsabG0md6_8g@mail.gmail.com>
<[email protected]>
<CAD21AoAO3zyooPQ+E2rpEi1YBc+gxZr0CUtJwxaFJ-WWimJNvA@mail.gmail.com>
<ZwY68OhKx/[email protected]>
<CAD21AoCAEXmxaDHAjMwqcpV6JVa91wBbXamQbk-rv1QnYhzGhA@mail.gmail.com>
<ZwdK1UYdLn/[email protected]>
<CAD21AoB-bxBqBEt9zbUQKTTUag1_NRPeT_5aGXC_j+z0+T3Hkw@mail.gmail.com>
<ZwfSXS/[email protected]>
<CAHut+PvRuPxs8103CSO_n_qu0BYJjJAin8yML3Go+Ur4a8tyWg@mail.gmail.com>
Hi,
On Fri, Oct 11, 2024 at 12:59:33PM +1100, Peter Smith wrote:
> Hi, Here are a few comments for patch set v13*
Thanks for looking at it.
> //////////
>
> Patch v13-0001
>
> ======
> Commit message
>
> 1.1
> /were no use case/was no use case/
Updated in v14 just shared up-thread.
> ~~~
>
> 1.2
> It seemed a bit odd that the switch cases for
> 'construct_array_builtin' are not the same as those for
> 'deconstruct_array_builtin'.
>
> For example, all these ones seem missing from deconstruct:
> case INT4OID:
> case INT8OID:
> case NAMEOID:
> case REGTYPEOID:
>
> I know that has nothing to do with your patch, and I guess it does not
> cause any problems otherwise there would be ERRORs. But, if you are to
> follow this same current pattern, then perhaps you don't need to add
> your new case for 'deconstruct_array_builtin', since AFAICT you are
> never using it.
That's right. Strict pairing between deconstruct_array_builtin() and
construct_array_builtin() is not required, let's remove this extra switch in
deconstruct_array_builtin() for code consistency (done in v14).
> //////////
>
> Patch v13-0002
>
> ======
> pg_get_logical_snapshot_meta:
>
> 2.1
> + Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
> + bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
>
> FWIW, if you wanted to avoid a few lines you could initialise the
> nulls array during the declaration.
> bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
>
> This seems a common pattern in other source code, and it replaces the
> need for the subsequent memset.
Okay, fine by me, let's do it for the "values" too in passing (this seems also
a common pattern), in v14.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
view thread (36+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add contrib/pg_logicalsnapinspect
In-Reply-To: <Zwklhj/[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox