MIME-Version: 1.0
References: <ZvRIZctV1M4PPmwT@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoBHkL-C22pgGJCA_8Jv6HjYq0c_1Y_b2Xktk7_SdUqxqA@mail.gmail.com>
 <ZwOkPDCKQ/gDNuZL@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAHut+PvtLb6g1L9C_zbufuHbJTWNq-BeseCpVgjsabG0md6_8g@mail.gmail.com>
 <ZwVc8SZWvTlzbE6x@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoAO3zyooPQ+E2rpEi1YBc+gxZr0CUtJwxaFJ-WWimJNvA@mail.gmail.com>
 <ZwY68OhKx/5jWeCj@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoCAEXmxaDHAjMwqcpV6JVa91wBbXamQbk-rv1QnYhzGhA@mail.gmail.com>
 <ZwdK1UYdLn/zFMHy@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoB-bxBqBEt9zbUQKTTUag1_NRPeT_5aGXC_j+z0+T3Hkw@mail.gmail.com> <ZwfSXS/3v73388jh@ip-10-97-1-34.eu-west-3.compute.internal>
In-Reply-To: <ZwfSXS/3v73388jh@ip-10-97-1-34.eu-west-3.compute.internal>
From: Peter Smith <smithpb2250@gmail.com>
Date: Fri, 11 Oct 2024 12:59:33 +1100
Message-ID: <CAHut+PvRuPxs8103CSO_n_qu0BYJjJAin8yML3Go+Ur4a8tyWg@mail.gmail.com>
Subject: Re: Add contrib/pg_logicalsnapinspect
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, 
	shveta malik <shveta.malik@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAHut%2BPvRuPxs8103CSO_n_qu0BYJjJAin8yML3Go%2BUr4a8tyWg%40mail.gmail.com>
Precedence: bulk

Hi, Here are a few comments for patch set v13*

//////////

Patch v13-0001

======
Commit message

1.1
/were no use case/was no use case/

~~~

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.

//////////

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.

~~~

pg_get_logical_snapshot_info:

2.2
+ Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS];
+ bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS];

Ditto of #2.1. You could instead just initialise in the declaration like:
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};

======
Kind Regards,
Peter Smith.
Fujitsu Australia