Re: Proposal: Make use of C99 designated initialisers for nulls/values arrays

On Wed, Oct 2, 2019 at 4:53 AM Smith, Peter <peters(at)fast(dot)au(dot)fujitsu(dot)com>
wrote:

> From: Isaac Morland <isaac(dot)morland(at)gmail(dot)com> Sent: Tuesday, 1 October
> 2019 11:32 PM
>
> >Typical Example:
> >Before:
> > Datum values[Natts_pg_attribute];
> > bool nulls[Natts_pg_attribute];
> > ...
> > memset(values, 0, sizeof(values));
> > memset(nulls, false, sizeof(nulls));
> >After:
> > Datum values[Natts_pg_attribute] = {0};
> > bool nulls[Natts_pg_attribute] = {0};
> >
> >I hope you'll forgive a noob question. Why does the "After"
> initialization for the boolean array have {0} rather than {false}?
>
> It is a valid question.
>
> I found that the original memsets that this patch replaces were already
> using 0 and false interchangeably. So I just picked one.
> Reasons I chose {0} over {false} are: (a) laziness, and (b) consistency
> with the values[] initialiser.
>

In this case, I think it is better to be consistent in all the places. As
of now (without patch), we are using 'false' or '0' to initialize the
boolean array. See below two instances from the patch:
1.
@@ -607,9 +601,9 @@ UpdateStatisticsForTypeChange(Oid statsOid, Oid
relationOid, int attnum,

Relation rel;

- Datum values[Natts_pg_statistic_ext_data];
- bool nulls[Natts_pg_statistic_ext_data];
- bool replaces[Natts_pg_statistic_ext_data];
+ Datum values[Natts_pg_statistic_ext_data] = {0};
+ bool nulls[Natts_pg_statistic_ext_data] = {0};
+ bool replaces[Natts_pg_statistic_ext_data] = {0};

oldtup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(statsOid));
if (!HeapTupleIsValid(oldtup))
@@ -630,10 +624,6 @@ UpdateStatisticsForTypeChange(Oid statsOid, Oid
relationOid, int attnum,
* OK, we need to reset some statistics. So let's build the new tuple,
* replacing the affected statistics types with NULL.
*/
- memset(nulls, 0, Natts_pg_statistic_ext_data * sizeof(bool));
- memset(replaces, 0, Natts_pg_statistic_ext_data * sizeof(bool));
- memset(values, 0, Natts_pg_statistic_ext_data * sizeof(Datum));

2.
@@ -69,10 +69,10 @@ CreateStatistics(CreateStatsStmt *stmt)
Oid namespaceId;
Oid stxowner = GetUserId();
HeapTuple htup;
- Datum values[Natts_pg_statistic_ext];
- bool nulls[Natts_pg_statistic_ext];
- Datum datavalues[Natts_pg_statistic_ext_data];
- bool datanulls[Natts_pg_statistic_ext_data];
+ Datum values[Natts_pg_statistic_ext] = {0};
+ bool nulls[Natts_pg_statistic_ext] = {0};
+ Datum datavalues[Natts_pg_statistic_ext_data] = {0};
+ bool datanulls[Natts_pg_statistic_ext_data] = {0};
int2vector *stxkeys;
Relation statrel;
Relation datarel;
@@ -330,9 +330,6 @@ CreateStatistics(CreateStatsStmt *stmt)
/*
* Everything seems fine, so let's build the pg_statistic_ext tuple.
*/
- memset(values, 0, sizeof(values));
- memset(nulls, false, sizeof(nulls));
-
statoid = GetNewOidWithIndex(statrel, StatisticExtOidIndexId,
Anum_pg_statistic_ext_oid);
values[Anum_pg_statistic_ext_oid - 1] = ObjectIdGetDatum(statoid);
@@ -357,9 +354,6 @@ CreateStatistics(CreateStatsStmt *stmt)
*/
datarel = table_open(StatisticExtDataRelationId, RowExclusiveLock);

- memset(datavalues, 0, sizeof(datavalues));
- memset(datanulls, false, sizeof(datanulls));

In the first usage, we are initializing the boolean array with 0 and in the
second case, we are using false. The patch changes it to use 0 at all the
places which I think is better.

I don't have any strong opinion on this, but I would mildly prefer to
initialize boolean array with false just for the sake of readability (we
generally initializing booleans with false).

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com