Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays - Mailing list pgsql-hackers
| From | Amit Kapila |
|---|---|
| Subject | Re: Proposal: Make use of C99 designated initialisers fornulls/values arrays |
| Date | |
| Msg-id | CAA4eK1JSA84LT73EFGhrF06-RRH=g74=oVMpBLZ2gFeOJZezKQ@mail.gmail.com Whole thread Raw |
| In response to | RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays ("Smith, Peter" <peters@fast.au.fujitsu.com>) |
| Responses |
RE: Proposal: Make use of C99 designated initialisers fornulls/values arrays
("Smith, Peter" <peters@fast.au.fujitsu.com>)
|
| List | pgsql-hackers |
On Wed, Oct 2, 2019 at 4:53 AM Smith, Peter <peters@fast.au.fujitsu.com> wrote:
From: Isaac Morland <isaac.morland@gmail.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));
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));
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).
pgsql-hackers by date: