Thread: 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
codefrom 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

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
existingcode 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

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
existingcode 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

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
> 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

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
canbe 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:

> 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: use
> explicit 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 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
> >

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
canbe 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
canbe 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
canbe 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
beforecan 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
beforecan 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
willbe 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
visualizeit'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
hasthe 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
sofar 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
commentlike the other places. What do you think ? 

Richard

--- On Tue, 14/6/11, Alvaro Herrera <alvherre@commandprompt.com> wrote:

> 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
>

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
hasthe 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