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

On Tue, Oct 8, 2019 at 4:43 AM Smith, Peter <peters(at)fast(dot)au(dot)fujitsu(dot)com> wrote:
>
> From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> Sent: Friday, 4 October 2019 4:50 PM
>
> >>How about I just define them both the same?
> >>#define INIT_ALL_ELEMS_ZERO {0}
> >>#define INIT_ALL_ELEMS_FALSE {0}
> >
> >I think using one define would be preferred, but you can wait and see if others prefer defining different macros for the same thing.
>
> While nowhere near unanimous, it seems majority favour using a macro (if only to protect the unwary and document the behaviour).
> And of those in favour of macros, using INIT_ALL_ELEMS_ZERO even for bool array is a clear preference.
>
> So, please find attached the updated patch, which now has just 1 macro.
Few thoughts on the patch:
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -770,8 +770,8 @@ pg_prepared_xact(PG_FUNCTION_ARGS)
GlobalTransaction gxact = &status->array[status->currIdx++];
PGPROC *proc = &ProcGlobal->allProcs[gxact->pgprocno];
PGXACT *pgxact = &ProcGlobal->allPgXact[gxact->pgprocno];
- Datum values[5];
- bool nulls[5];
+ Datum values[5] = INIT_ALL_ELEMS_ZERO;
+ bool nulls[5] = INIT_ALL_ELEMS_ZERO;
HeapTuple tuple;
Datum result;
Initialisation may not be required here as all the members are getting
populated immediately

@@ -1314,9 +1314,6 @@ SetDefaultACL(InternalDefaultACL *iacls)
Oid defAclOid;

/* Prepare to insert or update pg_default_acl entry */
- MemSet(values, 0, sizeof(values));
- MemSet(nulls, false, sizeof(nulls));
- MemSet(replaces, false, sizeof(replaces));

if (isNew)
We can place the comment just before the next block of code for
better readability like you have done in other places.

@@ -2024,9 +2018,6 @@ ExecGrant_Relation(InternalGrant *istmt)
nnewmembers = aclmembers(new_acl, &newmembers);

/* finished building new ACL value, now insert it */
- MemSet(values, 0, sizeof(values));
- MemSet(nulls, false, sizeof(nulls));
- MemSet(replaces, false, sizeof(replaces));

replaces[Anum_pg_class_relacl - 1] = true;
We can place the comment just before the next block of code for
better readability like you have done in other places.
There are few more instances like this in the same file, we can
handle that too.

-- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -77,7 +77,7 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
bool immediately_reserve = PG_GETARG_BOOL(1);
bool temporary = PG_GETARG_BOOL(2);
Datum values[2];
- bool nulls[2];
+ bool nulls[2] = INIT_ALL_ELEMS_ZERO;
TupleDesc tupdesc;
HeapTuple tuple;
Datum result;
@@ -95,12 +95,10 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
InvalidXLogRecPtr);

values[0] = NameGetDatum(&MyReplicationSlot->data.name);
- nulls[0] = false;

if (immediately_reserve)
{
values[1] = LSNGetDatum(MyReplicationSlot->data.restart_lsn);
- nulls[1] = false;
}
else
nulls[1] = true;
We might not gain much here, may be this change is not required.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com