| Lists: | pgsql-hackers |
|---|
| From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
|---|---|
| To: | richhguard-monotone(at)yahoo(dot)co(dot)uk |
| Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgreSQL(dot)org |
| Subject: | Re: PATCH: CreateComments: use explicit indexing for ``values'' |
| Date: | 2011-06-13 15:09:36 |
| Message-ID: | 18422.1307977776@sss.pgh.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
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
| From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | richhguard-monotone(at)yahoo(dot)co(dot)uk, pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: PATCH: CreateComments: use explicit indexing for ``values'' |
| Date: | 2011-06-13 16:18:43 |
| Message-ID: | BANLkTikD-dmyiik6zyp40ODgLs3L1WjQTw@mail.gmail.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
On Mon, Jun 13, 2011 at 11:09 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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