Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
On Jun14, 2011, at 17:47 , richhguard-monot...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
richhguard-monot...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
On Mon, Jun 13, 2011 at 4:10 PM, richhguard-monot...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
Alvaro Herrera alvhe...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
On Tue, Jun 14, 2011 at 10:30 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Alvaro Herrera alvhe...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
Excerpts from Tom Lane's message of mar jun 14 10:30:28 -0400 2011:
Alvaro Herrera alvhe...@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 alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
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
[HACKERS] 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 Richardcommit fd4f57d8e67d723c071bdc374ede58d453c854ca (master) Author: Richard Hopkins richhguard-monot...@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 */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
On Sun, Jun 12, 2011 at 7:26 AM, richhguard-monot...@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 loop The 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
Robert Haas robertmh...@gmail.com writes: On Sun, Jun 12, 2011 at 7:26 AM, richhguard-monot...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
On Mon, Jun 13, 2011 at 11:09 AM, Tom Lane t...@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 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? 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
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-monot...@yahoo.co.uk richhguard-monot...@yahoo.co.uk wrote: From: richhguard-monot...@yahoo.co.uk richhguard-monot...@yahoo.co.uk Subject: Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values'' To: Tom Lane t...@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 t...@sss.pgh.pa.us wrote: From: Tom Lane t...@sss.pgh.pa.us Subject: Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values'' To: richhguard-monot...@yahoo.co.uk Cc: Robert Haas robertmh...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
