Re: Refactoring proposal: initialize structures in a consistent way - Mailing list pgsql-hackers

> Hi,
>
> Very often we allocate a structure with palloc0() and then initialize
> its fields a second time with 0 / NULL / false / etc. For instance, in
> toasting.c we have:
>
> ```
>      indexInfo = makeNode(IndexInfo); // uses palloc0()
>      indexInfo->ii_NumIndexAttrs = 2;
>      indexInfo->ii_NumIndexKeyAttrs = 2;
>      indexInfo->ii_IndexAttrNumbers[0] = 1;
>      indexInfo->ii_IndexAttrNumbers[1] = 2;
>      indexInfo->ii_Expressions = NIL;
>      indexInfo->ii_ExpressionsState = NIL;
>      indexInfo->ii_Predicate = NIL;
>      indexInfo->ii_PredicateState = NULL;
>      indexInfo->ii_ExclusionOps = NULL;
>      indexInfo->ii_ExclusionProcs = NULL;
>      indexInfo->ii_ExclusionStrats = NULL;
>      indexInfo->ii_Unique = true;
>      indexInfo->ii_NullsNotDistinct = false;
>      indexInfo->ii_ReadyForInserts = true;
>      indexInfo->ii_CheckedUnchanged = false;
>      indexInfo->ii_IndexUnchanged = false;
>      indexInfo->ii_Concurrent = false;
>      indexInfo->ii_BrokenHotChain = false;
>      indexInfo->ii_ParallelWorkers = 0;
>      indexInfo->ii_Am = BTREE_AM_OID;
>      indexInfo->ii_AmCache = NULL;
>      indexInfo->ii_Context = CurrentMemoryContext;
> ```
>
> There are more complicated cases. For instance in execMain.c (and
> similarly elsewhere) we have:
>
> ```
>      rInfo = makeNode(ResultRelInfo);
>      InitResultRelInfo(rInfo,
>                        rel,
>                        0,        /* dummy rangetable index */
>                        rootRelInfo,
>                        estate->es_instrument);
> ```
>
> ... where InitResultRelInfo does:
>
> ```
>      MemSet(resultRelInfo, 0, sizeof(ResultRelInfo)); // !!!
>
>      resultRelInfo->type = T_ResultRelInfo;
>      resultRelInfo->ri_RangeTableIndex = resultRelationIndex;
>      resultRelInfo->ri_RelationDesc = resultRelationDesc;
>      resultRelInfo->ri_NumIndices = 0;
>      resultRelInfo->ri_IndexRelationDescs = NULL;
>      resultRelInfo->ri_IndexRelationInfo = NULL;
>      resultRelInfo->ri_needLockTagTuple =
>          IsInplaceUpdateRelation(resultRelationDesc);
> ```
>
> Here the fields are initialized 3 times in a row which is rather strange IMO.
>
> In the same time other places e.g. copy.c may just do:
>
> ```
>              select = makeNode(SelectStmt);
>              select->targetList = targetList;
>              select->fromClause = list_make1(from);
> ```
>
> IMO zeroing structures once is preferable in terms of both performance
> and readability. Unless there are strong objections I would like to
> propose a refactoring that would make the code consistent and use the
> pattern as in the latest example, perhaps with few exceptions e.g. for
> enum values that happened to be zero. Please let me know whether
> `false` and perhaps other values should be an exception too.
>
> If there are objections, I would like to know which method of
> initialization is preferable so that alternatively we could follow
> this pattern. Perhaps makeNode() shouldn't in fact use palloc0()? Or
> maybe we should generate functions like makeSelectStmt() and
> makeIndexInfo() ?
>
> Honestly I don't know which approach is best and whether this is even
> a problem we should invest our time into. Please let me know what you
> think.

Something to keep in mind is that palloc0() or memset(..., 0, ...) will also zero any padding bytes in the structure
whichsetting the individual fields will not. This may be important if the structure is written to disk for example.
 

I like setting fields that are expected to be zero by the code to zero explicitly for clarity. And also I would prefer
toonly not set the padding to 0 when absolutely sure the structure will never be directly written anywhere were we
don'twant to risk leaking data.
 

-- 
Anders Åstrand
Percona