Re: 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-monotone(at)yahoo(dot)co(dot)uk <richhguard-monotone(at)yahoo(dot)co(dot)uk> wrote:

> From: richhguard-monotone(at)yahoo(dot)co(dot)uk <richhguard-monotone(at)yahoo(dot)co(dot)uk>
> Subject: Re: [HACKERS] PATCH: CreateComments: use explicit indexing for ``values''
> To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us>
> wrote:
>
> > From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> > Subject: Re: [HACKERS] PATCH: CreateComments: use
> explicit indexing for ``values''
> > To: richhguard-monotone(at)yahoo(dot)co(dot)uk
> > Cc: "Robert Haas" <robertmhaas(at)gmail(dot)com>,
> pgsql-hackers(at)postgreSQL(dot)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(at)yahoo(dot)co(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support