Обсуждение: Mark deprecated operators as such in their comments?
I finally got around to completing the function-comments cleanup proposed here: http://archives.postgresql.org/pgsql-docs/2010-10/msg00041.php There are now a heck of a lot of boilerplate comments likeDESCR("implementation of + operator"); in pg_proc.h (about 700 of 'em to be exact). My original plan had involved getting initdb to generate those comments automatically instead of having to maintain them manually, but I desisted from that after noticing that there are various cases where we have multiple operators linking to the same pg_proc entry, so initdb wouldn't know which one to pick. But thinking about it this morning, I realize that all those cases are ones where we've replaced an old spelling of an operator name with a better choice, and really the old entry is deprecated but we still have it for compatibility reasons. So we could teach initdb how to build the desired comments if there were some easy way for it to recognize the deprecated operators. The obvious way to do that is to put something like "deprecated, use <@ instead" in the comment for the deprecated version. This seems like a good idea from a user's standpoint too, considering that the entire motivation for this effort was to ensure that \df (and by extension \do) output will tell you to avoid non-preferred ways of spelling a function/operator call. (BTW, the operators in question are @, ~, and @@@ uses that are now preferred to be spelled <@, @>, and @@ respectively.) So, two questions: 1. Do people like the idea of marking obsolete operator names this way? If so, exactly how to mark them? We could try to add "(deprecated, ...)" at the end of the existing description, or just replace the description completely. In some of these cases the existing description is pretty long, making the latter attractive. 2. Given that we do #1, is it really a good idea to generate the boilerplate comments automatically? The argument I can see against it is that right now there's a pretty simple coding rule "every pg_proc.h entry should have a comment". This is less confusing than "every pg_proc.h entry should have a comment, except those that are linked to pg_operator entries and aren't meant to be used directly". I'm not sure that argument outweighs "writing the boilerplate comment is a PITA", but I'm not sure it doesn't either. Comments? regards, tom lane
On Thu, Mar 3, 2011 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I finally got around to completing the function-comments cleanup > proposed here: > http://archives.postgresql.org/pgsql-docs/2010-10/msg00041.php > > There are now a heck of a lot of boilerplate comments like > DESCR("implementation of + operator"); > in pg_proc.h (about 700 of 'em to be exact). My original plan had > involved getting initdb to generate those comments automatically > instead of having to maintain them manually, but I desisted from > that after noticing that there are various cases where we have > multiple operators linking to the same pg_proc entry, so initdb > wouldn't know which one to pick. > > But thinking about it this morning, I realize that all those cases > are ones where we've replaced an old spelling of an operator name > with a better choice, and really the old entry is deprecated but > we still have it for compatibility reasons. So we could teach > initdb how to build the desired comments if there were some easy > way for it to recognize the deprecated operators. The obvious > way to do that is to put something like "deprecated, use <@ instead" > in the comment for the deprecated version. This seems like a > good idea from a user's standpoint too, considering that the entire > motivation for this effort was to ensure that \df (and by extension > \do) output will tell you to avoid non-preferred ways of spelling > a function/operator call. > > (BTW, the operators in question are @, ~, and @@@ uses that are > now preferred to be spelled <@, @>, and @@ respectively.) > > So, two questions: > > 1. Do people like the idea of marking obsolete operator names this > way? If so, exactly how to mark them? We could try to add > "(deprecated, ...)" at the end of the existing description, or just > replace the description completely. In some of these cases the > existing description is pretty long, making the latter attractive. "Deprecated, use <blah> instead"? > 2. Given that we do #1, is it really a good idea to generate the > boilerplate comments automatically? The argument I can see against it > is that right now there's a pretty simple coding rule "every pg_proc.h > entry should have a comment". This is less confusing than "every > pg_proc.h entry should have a comment, except those that are linked to > pg_operator entries and aren't meant to be used directly". I'm not > sure that argument outweighs "writing the boilerplate comment is a > PITA", but I'm not sure it doesn't either. I think the chances that future patches will follow the more complex coding rule are near zero, absent some type of automated enforcement mechanism. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Mar 3, 2011 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2. Given that we do #1, is it really a good idea to generate the
>> boilerplate comments automatically? �The argument I can see against it
>> is that right now there's a pretty simple coding rule "every pg_proc.h
>> entry should have a comment". �This is less confusing than "every
>> pg_proc.h entry should have a comment, except those that are linked to
>> pg_operator entries and aren't meant to be used directly". �I'm not
>> sure that argument outweighs "writing the boilerplate comment is a
>> PITA", but I'm not sure it doesn't either.
> I think the chances that future patches will follow the more complex
> coding rule are near zero, absent some type of automated enforcement
> mechanism.
Well, there is an enforcement mechanism: the regression tests will now
complain if any pg_proc.h entry lacks a comment. What they can't do
very well is enforce that the comment is sanely chosen. In particular
the likely failure mechanism is that someone submits a custom comment
for a function that would be better off being labeled as "implementation
of XXX operator". But AFAICS such a mistake is about equally likely
with either approach, maybe even a tad more so if submitters are forced
to comment every function instead of having an automatic default.
regards, tom lane
On Thu, Mar 3, 2011 at 3:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > 1. Do people like the idea of marking obsolete operator names this > way? If so, exactly how to mark them? We could try to add > "(deprecated, ...)" at the end of the existing description, or just > replace the description completely. In some of these cases the > existing description is pretty long, making the latter attractive. Marking deprecated legacy names is nice. I hate as a programmer being confronted with multiple identical-sounding options and not knowing which one I should be using. > 2. Given that we do #1, is it really a good idea to generate the > boilerplate comments automatically? The argument I can see against it > is that right now there's a pretty simple coding rule "every pg_proc.h > entry should have a comment". This is less confusing than "every > pg_proc.h entry should have a comment, except those that are linked to > pg_operator entries and aren't meant to be used directly". I'm not > sure that argument outweighs "writing the boilerplate comment is a > PITA", but I'm not sure it doesn't either. > A way out might be to have a token in the DESCR like "-" or "$opr" or something which indicates "substitute the default description here". But I'm not sure it's worth bothering. Filling in the description field is hardly the most annoying part of adding pg_proc entries for operators. If we could move most or all of the entries to an SQL file so that we didn't have to deal with commutator and negator oids and all that, that would save a lot of pain. -- greg
I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> "Deprecated, use <blah> instead"?
Everybody seems happy with that part of the proposal, so I'll make it
happen.
>> I think the chances that future patches will follow the more complex
>> coding rule are near zero, absent some type of automated enforcement
>> mechanism.
> Well, there is an enforcement mechanism: the regression tests will now
> complain if any pg_proc.h entry lacks a comment. What they can't do
> very well is enforce that the comment is sanely chosen. In particular
> the likely failure mechanism is that someone submits a custom comment
> for a function that would be better off being labeled as "implementation
> of XXX operator". But AFAICS such a mistake is about equally likely
> with either approach, maybe even a tad more so if submitters are forced
> to comment every function instead of having an automatic default.
After further reflection I think that it should be marginally less
error-prone to provide the default comment mechanism. So unless someone
feels more strongly against it than they've indicated so far, I'll go
ahead and do that.
regards, tom lane
Excerpts from Greg Stark's message of jue mar 03 13:02:53 -0300 2011: > But I'm not sure it's worth bothering. Filling in the description > field is hardly the most annoying part of adding pg_proc entries for > operators. If we could move most or all of the entries to an SQL file > so that we didn't have to deal with commutator and negator oids and > all that, that would save a lot of pain. You seem to want to have a completely new way to describe contents of pg_proc.h and pg_operator.h, from which the DATA and DESCR lines could be generated. Perhaps that's a worthy goal, not sure. I'm not sure it can be done with SQL though. -- Á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 Greg Stark's message of jue mar 03 13:02:53 -0300 2011:
>> But I'm not sure it's worth bothering. Filling in the description
>> field is hardly the most annoying part of adding pg_proc entries for
>> operators. If we could move most or all of the entries to an SQL file
>> so that we didn't have to deal with commutator and negator oids and
>> all that, that would save a lot of pain.
> You seem to want to have a completely new way to describe contents of
> pg_proc.h and pg_operator.h, from which the DATA and DESCR lines could
> be generated. Perhaps that's a worthy goal, not sure. I'm not sure it
> can be done with SQL though.
We've been around on that before. It'd probably be possible to reduce
pg_proc.h and pg_operator.h to some minimal set of functions/operators
needed to support the indexes on those catalogs, and then fill in the
rest using a higher-level notation; analogous to the difference in the
way we treat the bootstrap catalogs versus the rest of the catalogs.
I'm not really convinced that it's worth the trouble though. Making
up those DATA lines is not the hardest part of making a new built-in
function or operator; nor have I seen many errors in patches that
I think would have been avoided by a higher-level notation. (If memory
serves, by far the most common mistake in submissions against pg_proc.h
is failure to think carefully about the function's volatility marking.
A higher-level notation would help that not a whit.)
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > I'm not really convinced that it's worth the trouble though. Making > up those DATA lines is not the hardest part of making a new built-in No but it's cumbersome. I'd welcome simplification here, even if to be honest that itch isn't scratching me enough, some others are much pressing. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support