Thread: Operator Comments
During some testing of pgAdmin's internals whilst adding schema support I noticed that altering or setting a comment on an operator actually sets the comment on the operator function. In other words, change the comment on testschema.+(int4, int4) and the comment is actually set on the function pg_catalog.int4pl(int4, int4). Is this behaviour correct? I would have expected the pg_description entry for the comment to reference the oid of the operator itself, so each operator and int4pl(int4, int4) can all have distinct comments. Regards Dave.
Indeed... Comment on operator adds the comment to the procedures, and drop operator removes comments from pg_operator, leaving left over entries in pg_description. Looks like CommentOperator goes to quite a bit of work (5 lines) to accomplish fetching the procedure and states specifically it's not a bug. In which case RemoveOperator needs to drop comments by the procID as well. -- Rod ----- Original Message ----- From: "Dave Page" <dpage@vale-housing.co.uk> To: <pgsql-hackers@postgresql.org> Sent: Sunday, May 12, 2002 5:03 PM Subject: [HACKERS] Operator Comments > During some testing of pgAdmin's internals whilst adding schema support > I noticed that altering or setting a comment on an operator actually > sets the comment on the operator function. > > In other words, change the comment on testschema.+(int4, int4) and the > comment is actually set on the function pg_catalog.int4pl(int4, int4). > > Is this behaviour correct? I would have expected the pg_description > entry for the comment to reference the oid of the operator itself, so > each operator and int4pl(int4, int4) can all have distinct comments. > > Regards Dave. > > ---------------------------(end of broadcast)--------------------------- > TIP 3: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly >
"Rod Taylor" <rbt@zort.ca> writes:
> Looks like CommentOperator goes to quite a bit of work (5 lines) to
> accomplish fetching the procedure and states specifically it's not a
> bug.
Yeah, someone once thought it was a good idea, but I was wondering about
the wisdom of it just the other day. Currently this "feature" presents
a hole in the security of comments on functions: anyone can make an
operator referencing a function, and then they'll be allowed to set the
function's comment :-(.
I can see the value in having the function comment shown when there is
no comment specifically for the operator ... but perhaps that ought to
be implemented in the client requesters, rather than wired into the
catalog representation.
> In which case RemoveOperator needs to drop comments by the
> procID as well.
No, because the comment really belongs to the function and should go
away only when the function does. But I'd vote for giving operators
their own comments.
regards, tom lane
> "Rod Taylor" <rbt@zort.ca> writes: > > Looks like CommentOperator goes to quite a bit of work (5 lines) to > > accomplish fetching the procedure and states specifically it's not a > > bug. > > I can see the value in having the function comment shown when there is > no comment specifically for the operator ... but perhaps that ought to > be implemented in the client requesters, rather than wired into the > catalog representation. Agreed. If no-one objects, I'll submit a patch which makes comment on operator actually comment on the operator. It'll also coalesce(operator comment, function comment) in psql.
Tom Lane wrote: > > "Rod Taylor" <rbt@zort.ca> writes: > > Looks like CommentOperator goes to quite a bit of work (5 lines) to > > accomplish fetching the procedure and states specifically it's not a > > bug. > > Yeah, someone once thought it was a good idea, but I was wondering about > the wisdom of it just the other day. Currently this "feature" presents > a hole in the security of comments on functions: anyone can make an > operator referencing a function, and then they'll be allowed to set the > function's comment :-(. > > I can see the value in having the function comment shown when there is > no comment specifically for the operator ... but perhaps that ought to > be implemented in the client requesters, rather than wired into the > catalog representation. > > > In which case RemoveOperator needs to drop comments by the > > procID as well. > > No, because the comment really belongs to the function and should go > away only when the function does. But I'd vote for giving operators > their own comments. Here's the history, FWIW: I implemented COMMENT ON for just TABLES and COLUMNS, like Oracle. Bruce requested it for all objects I extended for all objects - including databases (my bad) ;-) Peter E. was rewriting psql and wanted the COMMENT on operators to reflect a COMMENT on the underlying function I submitted a patch to do that - I just do what I'm told ;-) Mike Mascari mascarm@mascari.com
Mike Mascari wrote: > Here's the history, FWIW: > > I implemented COMMENT ON for just TABLES and COLUMNS, like Oracle. > > Bruce requested it for all objects > > I extended for all objects - including databases (my bad) ;-) > > Peter E. was rewriting psql and wanted the COMMENT on operators to > reflect a COMMENT on the underlying function > > I submitted a patch to do that - I just do what I'm told ;-) Actually, the use of function comments for operators goes back to when I added comments to system tables in include/catalog. I wanted to avoid duplication of comments so I placed them only on the functions and let the operators display the function comments. Were there cases where we don't want the function comments for certain operators? I never anticipated that. Anyway, I looked at the new psql code and it works fine, tries pg_operator description first, then pg_proc if missing. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> -----Original Message----- > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > Sent: 05 June 2002 21:00 > To: Mike Mascari > Cc: Rod Taylor; Dave Page; pgsql-hackers@postgresql.org > Subject: Re: [HACKERS] Operator Comments > > > Mike Mascari wrote: > > Here's the history, FWIW: > > > > I implemented COMMENT ON for just TABLES and COLUMNS, like Oracle. > > > > Bruce requested it for all objects > > > > I extended for all objects - including databases (my bad) ;-) > > > > Peter E. was rewriting psql and wanted the COMMENT on operators to > > reflect a COMMENT on the underlying function > > > > I submitted a patch to do that - I just do what I'm told ;-) > > Actually, the use of function comments for operators goes > back to when I added comments to system tables in > include/catalog. I wanted to avoid duplication of comments > so I placed them only on the functions and let the operators > display the function comments. Were there cases where we > don't want the function comments for certain operators? I > never anticipated that. > > Anyway, I looked at the new psql code and it works fine, > tries pg_operator description first, then pg_proc if missing. The problem that I found was that if you update the comment on an operator (a trivial task in pgAdmin which is what I was coding at the time) it updates the comment on the underlying function - not so good as the new comment may no longer make sense when read from the perspective of the function. Of course, if the function can be used by different operators or even for other uses, then this situation is more likely to occur. Defaulting to the functions comment sounds OK, but I think an update should be stored against the operators oid, not the functions. Regards, Dave.
Dave Page wrote: > The problem that I found was that if you update the comment on an > operator (a trivial task in pgAdmin which is what I was coding at the > time) it updates the comment on the underlying function - not so good as > the new comment may no longer make sense when read from the perspective > of the function. Of course, if the function can be used by different > operators or even for other uses, then this situation is more likely to > occur. > > Defaulting to the functions comment sounds OK, but I think an update > should be stored against the operators oid, not the functions. Yes, agreed. Operator-specific comments are better, if that is what is specirfied by the user. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026