PATCH: CreateComments: use explicit indexing for ``values''
Hello,
I'm new to PostgreSQL and git, but having read through the wiki entries such as http://wiki.postgresql.org/wiki/Submitting_a_Patch, I think I have a patch worthy of submission.
It's a readability improvement in src/backend/commands/comment.c (CreateComments function), which changes the existing code from incrementing a variable for use as the array index, to use explicit ``values'' instead.
This has the following benefits
1) The structure of ``values'' is now clear at first glance.
2) ``i'' is then only used for 1 reason; the for loop
The patch is based on "master", and all existing tests pass.
Regards
Richard
Attachments:
create-comment-explicit-indexing-v1.patchtext/x-patch; name=create-comment-explicit-indexing-v1.patchDownload
commit fd4f57d8e67d723c071bdc374ede58d453c854ca (master)
Author: Richard Hopkins <richhguard-monotone@yahoo.co.uk>
Date: Sun Jun 12 12:03:28 2011 +0100
CreateComment: use explicit indexing for ``values''
This improves readability, and clarifies the structure of ``values''. No
functionality has changed, and all existing tests pass.
diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index d09bef0..20603dc 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[0] = ObjectIdGetDatum(oid);
+ values[1] = ObjectIdGetDatum(classoid);
+ values[2] = Int32GetDatum(subid);
+ values[3] = CStringGetTextDatum(comment);
}
/* Use the index to search for a matching old tuple */
On Sun, Jun 12, 2011 at 7:26 AM, <richhguard-monotone@yahoo.co.uk> wrote:
Hello,
I'm new to PostgreSQL and git, but having read through the wiki entries such as http://wiki.postgresql.org/wiki/Submitting_a_Patch, I think I have a patch worthy of submission.It's a readability improvement in src/backend/commands/comment.c (CreateComments function), which changes the existing code from incrementing a variable for use as the array index, to use explicit ``values'' instead.
This has the following benefits
1) The structure of ``values'' is now clear at first glance.
2) ``i'' is then only used for 1 reason; the for loopThe patch is based on "master", and all existing tests pass.
Wow. That code is pretty ugly, all right. I think, though, that we
probably ought to be using the Apg_description_<columnname> constants
instead of writing 0-3. Care to update the patch?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Jun 12, 2011 at 7:26 AM, <richhguard-monotone@yahoo.co.uk> wrote:
It's a readability improvement in src/backend/commands/comment.c (CreateComments function), which changes the existing code from incrementing a variable for use as the array index, to use explicit ``values'' instead.
Wow. That code is pretty ugly, all right. I think, though, that we
probably ought to be using the Apg_description_<columnname> constants
instead of writing 0-3. Care to update the patch?
Historically this i++ approach has been used in a lot of places that
fill in system catalog tuples. We've fixed some of them over time, but
I doubt this is the only one remaining. If we're going to try to remove
it here, maybe we ought to try to fix them all rather than just this
one. I agree that the main point of doing so would be to introduce the
greppable Apg_xxx constants, and so just using hard-coded integers is
not much of an improvement.
regards, tom lane
I wrote:
Historically this i++ approach has been used in a lot of places that
fill in system catalog tuples. We've fixed some of them over
time, but I doubt this is the only one remaining. If we're going
to try to remove it here, maybe we ought to try to fix them all
rather than just this one.
A quick grep reveals that the places that still do it that way are
OperatorShellMake
OperatorCreate
TypeShellMake
TypeCreate
update_attstats (though this one might be hard to improve)
CreateComments
CreateSharedComments
InsertRule
Of these, all but the two in comment.c follow the convention of
mentioning the assigned-to column in a comment, so that the code
is at least somewhat greppable. So those two definitely need
improvement, but should we consider changing the others while at it?
BTW, there are some contrib modules with functions-returning-record that
fill in result tuples this way as well. But we don't have symbolic
constants for the column numbers there, and it's probably not worth
introducing such.
regards, tom lane
Import Notes
Reply to msg id not found: 318629.71106.qm@web86704.mail.ird.yahoo.comReference msg id not found: 318629.71106.qm@web86704.mail.ird.yahoo.com | Resolved by subject fallback
On Mon, Jun 13, 2011 at 11:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Historically this i++ approach has been used in a lot of places that
fill in system catalog tuples. We've fixed some of them over
time, but I doubt this is the only one remaining. If we're going
to try to remove it here, maybe we ought to try to fix them all
rather than just this one.A quick grep reveals that the places that still do it that way are
OperatorShellMake
OperatorCreate
TypeShellMake
TypeCreate
update_attstats (though this one might be hard to improve)
CreateComments
CreateSharedComments
InsertRuleOf these, all but the two in comment.c follow the convention of
mentioning the assigned-to column in a comment, so that the code
is at least somewhat greppable. So those two definitely need
improvement, but should we consider changing the others while at it?
Have at it, if you like.
BTW, there are some contrib modules with functions-returning-record that
fill in result tuples this way as well. But we don't have symbolic
constants for the column numbers there, and it's probably not worth
introducing such.
Yeah, I think that would not be a good use of time.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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.
Again, this is based on master and all existing tests pass.
Regards
Richard
--- On Mon, 13/6/11, richhguard-monotone@yahoo.co.uk <richhguard-monotone@yahoo.co.uk> wrote:Show quoted text
From: richhguard-monotone@yahoo.co.uk <richhguard-monotone@yahoo.co.uk>
Subject: Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
To: "Tom Lane" <tgl@sss.pgh.pa.us>
Date: Monday, 13 June, 2011, 21:08
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.Again, this is based on master and all existing tests
pass.Regards
Richard--- On Mon, 13/6/11, Tom Lane <tgl@sss.pgh.pa.us> wrote:From: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: [HACKERS] PATCH: CreateComments: useexplicit indexing for ``values''
To: richhguard-monotone@yahoo.co.uk
Cc: "Robert Haas" <robertmhaas@gmail.com>,pgsql-hackers@postgreSQL.org
Date: Monday, 13 June, 2011, 16:09
I wrote:Historically this i++ approach has been used
in a
lot of places that
fill in system catalog tuples. We've fixed
some of them over
time, but I doubt this is the only one
remaining. If we're going
to try to remove it here, maybe we ought to
try to
fix them all
rather than just this one.
A quick grep reveals that the places that still do it
that
way are
OperatorShellMake
OperatorCreate
TypeShellMake
TypeCreate
update_attstats (though this one might be hard toimprove)
CreateComments
CreateSharedComments
InsertRuleOf these, all but the two in comment.c follow the
convention of
mentioning the assigned-to column in a comment, sothat the
code
is at least somewhat greppable. So those two
definitely need
improvement, but should we consider changing theothers
while at it?
BTW, there are some contrib modules with
functions-returning-record that
fill in result tuples this way as well. But wedon't
have symbolic
constants for the column numbers there, and it'sprobably
not worth
introducing such.
regards, tom lane
Import Notes
Resolved by subject fallback
On Mon, Jun 13, 2011 at 4:10 PM, <richhguard-monotone@yahoo.co.uk> wrote:
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.
Again, this is based on master and all existing tests pass.
Anything you're not sure about, just leave alone.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from richhguard-monotone's message of lun jun 13 16:10:17 -0400 2011:
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.
He already did no?
I did think of a possible way to rewrite update_attstats: instead of
for (k = 0; k < STATISTIC_NUM_SLOTS; k++)
{
values[i++] = ObjectIdGetDatum(stats->staop[k]); /* staopN */
}
do
for (k = 0; k < STATISTIC_NUM_SLOTS; k++)
{
values[Anum_pg_statistic_staop1 - 1 + k] = ObjectIdGetDatum(stats->staop[k]);
}
etc. However, it's not clear to me whether this is really an
improvement. Opinions?
regards, tom lane
On Tue, Jun 14, 2011 at 10:30 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from richhguard-monotone's message of lun jun 13 16:10:17 -0400 2011:
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.He already did no?
I did think of a possible way to rewrite update_attstats: instead of
for (k = 0; k < STATISTIC_NUM_SLOTS; k++)
{
values[i++] = ObjectIdGetDatum(stats->staop[k]); /* staopN */
}do
for (k = 0; k < STATISTIC_NUM_SLOTS; k++)
{
values[Anum_pg_statistic_staop1 - 1 + k] = ObjectIdGetDatum(stats->staop[k]);
}etc. However, it's not clear to me whether this is really an
improvement. Opinions?
I don't care that much, but IMV that's just gilding the lily.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Tom Lane's message of mar jun 14 10:30:28 -0400 2011:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from richhguard-monotone's message of lun jun 13 16:10:17 -0400 2011:
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.He already did no?
I don't see the patch attached anywhere ...
I did think of a possible way to rewrite update_attstats: instead of
for (k = 0; k < STATISTIC_NUM_SLOTS; k++)
{
values[i++] = ObjectIdGetDatum(stats->staop[k]); /* staopN */
}do
for (k = 0; k < STATISTIC_NUM_SLOTS; k++)
{
values[Anum_pg_statistic_staop1 - 1 + k] = ObjectIdGetDatum(stats->staop[k]);
}etc. However, it's not clear to me whether this is really an
improvement. Opinions?
I guess the other option is
i = Anum_pg_statistic_staop1 - 1;
for (k = 0; k < STATISTIC_NUM_SLOTS; k++)
{
values[i++] = ObjectIdGetDatum(stats->staop[k]);
}
(I also tried moving the i initialization to the "for" first arg, but it
seems better this way)
Not sure what's better.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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 <alvherre@commandprompt.com> wrote:Show quoted text
From: Alvaro Herrera <alvherre@commandprompt.com>
Subject: Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
To: "richhguard-monotone" <richhguard-monotone@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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development,
24x7 support
Attachments:
initialize-arrays-symbolic-constants-v3.patchtext/x-patch; name=initialize-arrays-symbolic-constants-v3.patchDownload
commit 7a689fc96b624de5de589cd9e2ef1c42ae542a16
Author: Richard Hopkins <richhguard-monotone@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
Import Notes
Resolved by subject fallback
On Jun14, 2011, at 17:47 , richhguard-monotone@yahoo.co.uk wrote:
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.
Here's a review of that patch.
The patch applies cleanly to HEAD, looks correct, and passes "make check".
The patch makes the code far more readable and makes adding new columns
to one of the affected system catalogs less error-prone.
Since the chance of us ever back-patching changes to the system catalog's
structure, the patch doesn't introduce additional back-patching hurdles.
I'm thus marking this Read for Committer.
best regards,
Florian Pflug
richhguard-monotone@yahoo.co.uk writes:
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.
Applied, along with changing update_attstats() using Alvaro's idea.
regards, tom lane