I have left update_attstat which I'm unsure about, and have attached the
updated patch handling the other cases. This will be linked in via the
commitfest page.
I picked commands/comment.c randomly and found the i = 0, i++ method of
initializing the array made it harder for me to visualize it's structure, or
verify each value was in the correct place in the array.
Obviously we can assume each value is in the correct place because it's been
like that in working code.
This patch makes the intent of each initialization clear by using the constants
directly instead of in a comment, and has the effect of being able to verify
each line on it's own. The original requires verification of the preceding
i++'s.
I expanded upon my original patch of only handling CreateComments to include
the other cases for consistency - but only so far as update_attstats as I found
it too complex for me and left it. I don't like to break anything.
I'm not going to push for it if most people prefer the original code for
readability, or maintainability across versions; I just thought I'd post my
thoughts with a patch.
An easier route, might be for me to submit a patch which just changes comment.c
by adding in the constants via a comment like the other places. What do you
think ?
Richard
--- On Tue, 14/6/11, Alvaro Herrera <alvhe...@commandprompt.com> wrote:
> From: Alvaro Herrera <alvhe...@commandprompt.com>
> Subject: Re: [HACKERS] PATCH: CreateComments: use explicit indexing for
> ``values''
> To: "richhguard-monotone" <richhguard-monot...@yahoo.co.uk>
> Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>
> Date: Tuesday, 14 June, 2011, 15:20
> Excerpts from richhguard-monotone's
> message of lun jun 13 16:10:17 -0400 2011:
> > Apologies - I meant to CC in the list but forgot.
> >
> > I have gone through and changed all the related
> functions except ``update_attstats''.
> >
> > Do you have any advice of how to handle the inner
> loops, such as those initializing ``stakindN''. The entries
> before can be handled just like in this patch, by using the
> symbolic constants.
>
> Based on Tom's comments, I'd submit the patch without that
> bit, at least
> as a first step.
>
> > Again, this is based on master and all existing tests
> pass.
>
> Please post the patch and add it here:
> https://commitfest.postgresql.org/action/commitfest_view?id=10
>
> --
> Álvaro Herrera <alvhe...@commandprompt.com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development,
> 24x7 support
>
commit 7a689fc96b624de5de589cd9e2ef1c42ae542a16
Author: Richard Hopkins <richhguard-monot...@yahoo.co.uk>
Date: Mon Jun 13 20:44:15 2011 +0100
Initialize arrays using Anum_pg symbolic constants
This clarifies the intent of the code by using the already defined symbolic
constants; the constants were referenced in the related comments for each
column anyway.
However, ``update_attstats'' still uses the old style of i=0, and then i++
with each element initialization. I have left this for now as it seems too
tricky to include with these changes.
``update_attstats'' has a few loops using ``STATISTIC_NUM_SLOTS'' and I'm
unsure whether to remove them, and use the symbolic constants such as
``Anum_pg_statistic_stakind1'' at the expense of adding extra lines.
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index ccd0fe1..4d2d7b7 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -234,22 +234,21 @@ OperatorShellMake(const char *operatorName,
* initialize values[] with the operator name and input data types. Note
* that oprcode is set to InvalidOid, indicating it's a shell.
*/
- i = 0;
namestrcpy(&oname, operatorName);
- values[i++] = NameGetDatum(&oname); /* oprname */
- values[i++] = ObjectIdGetDatum(operatorNamespace); /* oprnamespace */
- values[i++] = ObjectIdGetDatum(GetUserId()); /* oprowner */
- values[i++] = CharGetDatum(leftTypeId ? (rightTypeId ? 'b' : 'r') : 'l'); /* oprkind */
- values[i++] = BoolGetDatum(false); /* oprcanmerge */
- values[i++] = BoolGetDatum(false); /* oprcanhash */
- values[i++] = ObjectIdGetDatum(leftTypeId); /* oprleft */
- values[i++] = ObjectIdGetDatum(rightTypeId); /* oprright */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* oprresult */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* oprcom */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* oprnegate */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* oprcode */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* oprrest */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* oprjoin */
+ values[Anum_pg_operator_oprname - 1] = NameGetDatum(&oname);
+ values[Anum_pg_operator_oprnamespace - 1] = ObjectIdGetDatum(operatorNamespace);
+ values[Anum_pg_operator_oprowner - 1] = ObjectIdGetDatum(GetUserId());
+ values[Anum_pg_operator_oprkind - 1] = CharGetDatum(leftTypeId ? (rightTypeId ? 'b' : 'r') : 'l');
+ values[Anum_pg_operator_oprcanmerge - 1] = BoolGetDatum(false);
+ values[Anum_pg_operator_oprcanhash - 1] = BoolGetDatum(false);
+ values[Anum_pg_operator_oprleft - 1] = ObjectIdGetDatum(leftTypeId);
+ values[Anum_pg_operator_oprright - 1] = ObjectIdGetDatum(rightTypeId);
+ values[Anum_pg_operator_oprresult - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_operator_oprcode - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_operator_oprrest - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_operator_oprjoin - 1] = ObjectIdGetDatum(InvalidOid);
/*
* open pg_operator
@@ -492,22 +491,21 @@ OperatorCreate(const char *operatorName,
nulls[i] = false;
}
- i = 0;
namestrcpy(&oname, operatorName);
- values[i++] = NameGetDatum(&oname); /* oprname */
- values[i++] = ObjectIdGetDatum(operatorNamespace); /* oprnamespace */
- values[i++] = ObjectIdGetDatum(GetUserId()); /* oprowner */
- values[i++] = CharGetDatum(leftTypeId ? (rightTypeId ? 'b' : 'r') : 'l'); /* oprkind */
- values[i++] = BoolGetDatum(canMerge); /* oprcanmerge */
- values[i++] = BoolGetDatum(canHash); /* oprcanhash */
- values[i++] = ObjectIdGetDatum(leftTypeId); /* oprleft */
- values[i++] = ObjectIdGetDatum(rightTypeId); /* oprright */
- values[i++] = ObjectIdGetDatum(operResultType); /* oprresult */
- values[i++] = ObjectIdGetDatum(commutatorId); /* oprcom */
- values[i++] = ObjectIdGetDatum(negatorId); /* oprnegate */
- values[i++] = ObjectIdGetDatum(procedureId); /* oprcode */
- values[i++] = ObjectIdGetDatum(restrictionId); /* oprrest */
- values[i++] = ObjectIdGetDatum(joinId); /* oprjoin */
+ values[Anum_pg_operator_oprname - 1] = NameGetDatum(&oname);
+ values[Anum_pg_operator_oprnamespace - 1] = ObjectIdGetDatum(operatorNamespace);
+ values[Anum_pg_operator_oprowner - 1] = ObjectIdGetDatum(GetUserId());
+ values[Anum_pg_operator_oprkind - 1] = CharGetDatum(leftTypeId ? (rightTypeId ? 'b' : 'r') : 'l');
+ values[Anum_pg_operator_oprcanmerge - 1] = BoolGetDatum(canMerge);
+ values[Anum_pg_operator_oprcanhash - 1] = BoolGetDatum(canHash);
+ values[Anum_pg_operator_oprleft - 1] = ObjectIdGetDatum(leftTypeId);
+ values[Anum_pg_operator_oprright - 1] = ObjectIdGetDatum(rightTypeId);
+ values[Anum_pg_operator_oprresult - 1] = ObjectIdGetDatum(operResultType);
+ values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(commutatorId);
+ values[Anum_pg_operator_oprnegate - 1] = ObjectIdGetDatum(negatorId);
+ values[Anum_pg_operator_oprcode - 1] = ObjectIdGetDatum(procedureId);
+ values[Anum_pg_operator_oprrest - 1] = ObjectIdGetDatum(restrictionId);
+ values[Anum_pg_operator_oprjoin - 1] = ObjectIdGetDatum(joinId);
pg_operator_desc = heap_open(OperatorRelationId, RowExclusiveLock);
diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index de5c63d..2ce1aa4 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -87,37 +87,36 @@ TypeShellMake(const char *typeName, Oid typeNamespace, Oid ownerId)
* give it typtype = TYPTYPE_PSEUDO as extra insurance that it won't be
* mistaken for a usable type.
*/
- i = 0;
namestrcpy(&name, typeName);
- values[i++] = NameGetDatum(&name); /* typname */
- values[i++] = ObjectIdGetDatum(typeNamespace); /* typnamespace */
- values[i++] = ObjectIdGetDatum(ownerId); /* typowner */
- values[i++] = Int16GetDatum(sizeof(int4)); /* typlen */
- values[i++] = BoolGetDatum(true); /* typbyval */
- values[i++] = CharGetDatum(TYPTYPE_PSEUDO); /* typtype */
- values[i++] = CharGetDatum(TYPCATEGORY_PSEUDOTYPE); /* typcategory */
- values[i++] = BoolGetDatum(false); /* typispreferred */
- values[i++] = BoolGetDatum(false); /* typisdefined */
- values[i++] = CharGetDatum(DEFAULT_TYPDELIM); /* typdelim */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typrelid */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typelem */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typarray */
- values[i++] = ObjectIdGetDatum(F_SHELL_IN); /* typinput */
- values[i++] = ObjectIdGetDatum(F_SHELL_OUT); /* typoutput */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typreceive */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typsend */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typmodin */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typmodout */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typanalyze */
- values[i++] = CharGetDatum('i'); /* typalign */
- values[i++] = CharGetDatum('p'); /* typstorage */
- values[i++] = BoolGetDatum(false); /* typnotnull */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typbasetype */
- values[i++] = Int32GetDatum(-1); /* typtypmod */
- values[i++] = Int32GetDatum(0); /* typndims */
- values[i++] = ObjectIdGetDatum(InvalidOid); /* typcollation */
- nulls[i++] = true; /* typdefaultbin */
- nulls[i++] = true; /* typdefault */
+ values[Anum_pg_type_typname - 1] = NameGetDatum(&name);
+ values[Anum_pg_type_typnamespace - 1] = ObjectIdGetDatum(typeNamespace);
+ values[Anum_pg_type_typowner - 1] = ObjectIdGetDatum(ownerId);
+ values[Anum_pg_type_typlen - 1] = Int16GetDatum(sizeof(int4));
+ values[Anum_pg_type_typbyval - 1] = BoolGetDatum(true);
+ values[Anum_pg_type_typtype - 1] = CharGetDatum(TYPTYPE_PSEUDO);
+ values[Anum_pg_type_typcategory - 1] = CharGetDatum(TYPCATEGORY_PSEUDOTYPE);
+ values[Anum_pg_type_typispreferred - 1] = BoolGetDatum(false);
+ values[Anum_pg_type_typisdefined - 1] = BoolGetDatum(false);
+ values[Anum_pg_type_typdelim - 1] = CharGetDatum(DEFAULT_TYPDELIM);
+ values[Anum_pg_type_typrelid - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typelem - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typarray - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typinput - 1] = ObjectIdGetDatum(F_SHELL_IN);
+ values[Anum_pg_type_typoutput - 1] = ObjectIdGetDatum(F_SHELL_OUT);
+ values[Anum_pg_type_typreceive - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typsend - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typmodin - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typmodout - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typanalyze - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typalign - 1] = CharGetDatum('i');
+ values[Anum_pg_type_typstorage - 1] = CharGetDatum('p');
+ values[Anum_pg_type_typnotnull - 1] = BoolGetDatum(false);
+ values[Anum_pg_type_typbasetype - 1] = ObjectIdGetDatum(InvalidOid);
+ values[Anum_pg_type_typtypmod - 1] = Int32GetDatum(-1);
+ values[Anum_pg_type_typndims - 1] = Int32GetDatum(0);
+ values[Anum_pg_type_typcollation - 1] = ObjectIdGetDatum(InvalidOid);
+ nulls[Anum_pg_type_typdefaultbin - 1] = true;
+ nulls[Anum_pg_type_typdefault - 1] = true;
/*
* create a new type tuple
@@ -324,54 +323,51 @@ TypeCreate(Oid newTypeOid,
/*
* initialize the *values information
*/
- i = 0;
namestrcpy(&name, typeName);
- values[i++] = NameGetDatum(&name); /* typname */
- values[i++] = ObjectIdGetDatum(typeNamespace); /* typnamespace */
- values[i++] = ObjectIdGetDatum(ownerId); /* typowner */
- values[i++] = Int16GetDatum(internalSize); /* typlen */
- values[i++] = BoolGetDatum(passedByValue); /* typbyval */
- values[i++] = CharGetDatum(typeType); /* typtype */
- values[i++] = CharGetDatum(typeCategory); /* typcategory */
- values[i++] = BoolGetDatum(typePreferred); /* typispreferred */
- values[i++] = BoolGetDatum(true); /* typisdefined */
- values[i++] = CharGetDatum(typDelim); /* typdelim */
- values[i++] = ObjectIdGetDatum(relationOid); /* typrelid */
- values[i++] = ObjectIdGetDatum(elementType); /* typelem */
- values[i++] = ObjectIdGetDatum(arrayType); /* typarray */
- values[i++] = ObjectIdGetDatum(inputProcedure); /* typinput */
- values[i++] = ObjectIdGetDatum(outputProcedure); /* typoutput */
- values[i++] = ObjectIdGetDatum(receiveProcedure); /* typreceive */
- values[i++] = ObjectIdGetDatum(sendProcedure); /* typsend */
- values[i++] = ObjectIdGetDatum(typmodinProcedure); /* typmodin */
- values[i++] = ObjectIdGetDatum(typmodoutProcedure); /* typmodout */
- values[i++] = ObjectIdGetDatum(analyzeProcedure); /* typanalyze */
- values[i++] = CharGetDatum(alignment); /* typalign */
- values[i++] = CharGetDatum(storage); /* typstorage */
- values[i++] = BoolGetDatum(typeNotNull); /* typnotnull */
- values[i++] = ObjectIdGetDatum(baseType); /* typbasetype */
- values[i++] = Int32GetDatum(typeMod); /* typtypmod */
- values[i++] = Int32GetDatum(typNDims); /* typndims */
- values[i++] = ObjectIdGetDatum(typeCollation); /* typcollation */
+ values[Anum_pg_type_typname - 1] = NameGetDatum(&name);
+ values[Anum_pg_type_typnamespace - 1] = ObjectIdGetDatum(typeNamespace);
+ values[Anum_pg_type_typowner - 1] = ObjectIdGetDatum(ownerId);
+ values[Anum_pg_type_typlen - 1] = Int16GetDatum(internalSize);
+ values[Anum_pg_type_typbyval - 1] = BoolGetDatum(passedByValue);
+ values[Anum_pg_type_typtype - 1] = CharGetDatum(typeType);
+ values[Anum_pg_type_typcategory - 1] = CharGetDatum(typeCategory);
+ values[Anum_pg_type_typispreferred - 1] = BoolGetDatum(typePreferred);
+ values[Anum_pg_type_typisdefined - 1] = BoolGetDatum(true);
+ values[Anum_pg_type_typdelim - 1] = CharGetDatum(typDelim);
+ values[Anum_pg_type_typrelid - 1] = ObjectIdGetDatum(relationOid);
+ values[Anum_pg_type_typelem - 1] = ObjectIdGetDatum(elementType);
+ values[Anum_pg_type_typarray - 1] = ObjectIdGetDatum(arrayType);
+ values[Anum_pg_type_typinput - 1] = ObjectIdGetDatum(inputProcedure);
+ values[Anum_pg_type_typoutput - 1] = ObjectIdGetDatum(outputProcedure);
+ values[Anum_pg_type_typreceive - 1] = ObjectIdGetDatum(receiveProcedure);
+ values[Anum_pg_type_typsend - 1] = ObjectIdGetDatum(sendProcedure);
+ values[Anum_pg_type_typmodin - 1] = ObjectIdGetDatum(typmodinProcedure);
+ values[Anum_pg_type_typmodout - 1] = ObjectIdGetDatum(typmodoutProcedure);
+ values[Anum_pg_type_typanalyze - 1] = ObjectIdGetDatum(analyzeProcedure);
+ values[Anum_pg_type_typalign - 1] = CharGetDatum(alignment);
+ values[Anum_pg_type_typstorage - 1] = CharGetDatum(storage);
+ values[Anum_pg_type_typnotnull - 1] = BoolGetDatum(typeNotNull);
+ values[Anum_pg_type_typbasetype - 1] = ObjectIdGetDatum(baseType);
+ values[Anum_pg_type_typtypmod - 1] = Int32GetDatum(typeMod);
+ values[Anum_pg_type_typndims - 1] = Int32GetDatum(typNDims);
+ values[Anum_pg_type_typcollation - 1] = ObjectIdGetDatum(typeCollation);
/*
* initialize the default binary value for this type. Check for nulls of
* course.
*/
if (defaultTypeBin)
- values[i] = CStringGetTextDatum(defaultTypeBin);
+ values[Anum_pg_type_typdefaultbin - 1] = CStringGetTextDatum(defaultTypeBin);
else
- nulls[i] = true;
- i++; /* typdefaultbin */
+ nulls[Anum_pg_type_typdefaultbin - 1] = true;
/*
* initialize the default value for this type.
*/
if (defaultTypeValue)
- values[i] = CStringGetTextDatum(defaultTypeValue);
+ values[Anum_pg_type_typdefault - 1] = CStringGetTextDatum(defaultTypeValue);
else
- nulls[i] = true;
- i++; /* typdefault */
+ nulls[Anum_pg_type_typdefault - 1] = true;
/*
* open pg_type and prepare to insert or update a row.
diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index d09bef0..587a689 100644
--- a/src/backend/commands/comment.c
+++ b/src/backend/commands/comment.c
@@ -157,11 +157,10 @@ CreateComments(Oid oid, Oid classoid, int32 subid, char *comment)
nulls[i] = false;
replaces[i] = true;
}
- i = 0;
- values[i++] = ObjectIdGetDatum(oid);
- values[i++] = ObjectIdGetDatum(classoid);
- values[i++] = Int32GetDatum(subid);
- values[i++] = CStringGetTextDatum(comment);
+ values[Anum_pg_description_objoid - 1] = ObjectIdGetDatum(oid);
+ values[Anum_pg_description_classoid - 1] = ObjectIdGetDatum(classoid);
+ values[Anum_pg_description_objsubid - 1] = Int32GetDatum(subid);
+ values[Anum_pg_description_description - 1] = CStringGetTextDatum(comment);
}
/* Use the index to search for a matching old tuple */
@@ -257,10 +256,9 @@ CreateSharedComments(Oid oid, Oid classoid, char *comment)
nulls[i] = false;
replaces[i] = true;
}
- i = 0;
- values[i++] = ObjectIdGetDatum(oid);
- values[i++] = ObjectIdGetDatum(classoid);
- values[i++] = CStringGetTextDatum(comment);
+ values[Anum_pg_shdescription_objoid - 1] = ObjectIdGetDatum(oid);
+ values[Anum_pg_shdescription_classoid - 1] = ObjectIdGetDatum(classoid);
+ values[Anum_pg_shdescription_description - 1] = CStringGetTextDatum(comment);
}
/* Use the index to search for a matching old tuple */
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 52c55de..59147e1 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -78,16 +78,15 @@ InsertRule(char *rulname,
*/
MemSet(nulls, false, sizeof(nulls));
- i = 0;
namestrcpy(&rname, rulname);
- values[i++] = NameGetDatum(&rname); /* rulename */
- values[i++] = ObjectIdGetDatum(eventrel_oid); /* ev_class */
- values[i++] = Int16GetDatum(evslot_index); /* ev_attr */
- values[i++] = CharGetDatum(evtype + '0'); /* ev_type */
- values[i++] = CharGetDatum(RULE_FIRES_ON_ORIGIN); /* ev_enabled */
- values[i++] = BoolGetDatum(evinstead); /* is_instead */
- values[i++] = CStringGetTextDatum(evqual); /* ev_qual */
- values[i++] = CStringGetTextDatum(actiontree); /* ev_action */
+ values[Anum_pg_rewrite_rulename - 1] = NameGetDatum(&rname);
+ values[Anum_pg_rewrite_ev_class - 1] = ObjectIdGetDatum(eventrel_oid);
+ values[Anum_pg_rewrite_ev_attr - 1] = Int16GetDatum(evslot_index);
+ values[Anum_pg_rewrite_ev_type - 1] = CharGetDatum(evtype + '0');
+ values[Anum_pg_rewrite_ev_enabled - 1] = CharGetDatum(RULE_FIRES_ON_ORIGIN);
+ values[Anum_pg_rewrite_is_instead - 1] = BoolGetDatum(evinstead);
+ values[Anum_pg_rewrite_ev_qual - 1] = CStringGetTextDatum(evqual);
+ values[Anum_pg_rewrite_ev_action - 1] = CStringGetTextDatum(actiontree);
/*
* Ready to store new pg_rewrite tuple
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
