Re: Patch: Code comments: why some text-handling functions are leakproof
I'm going to mark this returned with feedback. If you have a chance to update the patch moving the documentation to xfunc.sgml the way Tom describes make sure to create a new commitfest entry. I would suggest submitting the patch as a followup on this thread so when it's added to the commitfest it links to this whole discussion. On Mon, 28 Feb 2022 at 17:12, Tom Lane wrote: > > Robert Haas writes: > > On Tue, Jan 11, 2022 at 2:07 AM Gurjeet Singh wrote: > >> This is more or less a verbatim copy of Tom's comment in email thread at > >> [1]. > >> > >> I could not find an appropriate spot to place these comments, so I placed > >> them on bttextcmp() function, The only other place that I could see we can > >> place these comments is in the file src/backend/optimizer/README, because > >> there is some consideration given to leakproof functions in optimizer > >> docs. But these comments seem quite out of place in optimizer docs. > > > It doesn't seem particularly likely that someone who is thinking about > > changing this in the future would notice the comment in the place > > where you propose to put it, nor that they would read the optimizer > > README. > > Agreed. I think if we wanted to make an upgrade in the way function > leakproofness is documented, we ought to add a about it in > xfunc.sgml, adjacent to the one about function volatility categories. > This could perhaps consolidate some of the existing documentation mentions > of leakproofness, as well as adding text similar to what Gurjeet suggests. > > > Furthermore, I don't know that everyone agrees with Tom about this. I > > do agree that it's more important to mark relational operators > > leakproof than other things, and I also agree that conservatism is > > warranted. But that does not mean that someone could not make a > > compelling argument for marking other functions leakproof. > > ISTM the proposed text does a reasonable job of explaining why > we made the decisions currently embedded in pg_proc.proleakproof. > If we make some other decisions in future, updating the rationale > in the docs would be an appropriate part of that. > > regards, tom lane > > -- greg
Re: Patch: Code comments: why some text-handling functions are leakproof
Robert Haas writes: > On Tue, Jan 11, 2022 at 2:07 AM Gurjeet Singh wrote: >> This is more or less a verbatim copy of Tom's comment in email thread at [1]. >> >> I could not find an appropriate spot to place these comments, so I placed >> them on bttextcmp() function, The only other place that I could see we can >> place these comments is in the file src/backend/optimizer/README, because >> there is some consideration given to leakproof functions in optimizer docs. >> But these comments seem quite out of place in optimizer docs. > It doesn't seem particularly likely that someone who is thinking about > changing this in the future would notice the comment in the place > where you propose to put it, nor that they would read the optimizer > README. Agreed. I think if we wanted to make an upgrade in the way function leakproofness is documented, we ought to add a about it in xfunc.sgml, adjacent to the one about function volatility categories. This could perhaps consolidate some of the existing documentation mentions of leakproofness, as well as adding text similar to what Gurjeet suggests. > Furthermore, I don't know that everyone agrees with Tom about this. I > do agree that it's more important to mark relational operators > leakproof than other things, and I also agree that conservatism is > warranted. But that does not mean that someone could not make a > compelling argument for marking other functions leakproof. ISTM the proposed text does a reasonable job of explaining why we made the decisions currently embedded in pg_proc.proleakproof. If we make some other decisions in future, updating the rationale in the docs would be an appropriate part of that. regards, tom lane
Re: Patch: Code comments: why some text-handling functions are leakproof
On Tue, Jan 11, 2022 at 2:07 AM Gurjeet Singh wrote: > Please see attached a small patch to document why some text-processing > functions are marked as leakproof, while some others are not. > > This is more or less a verbatim copy of Tom's comment in email thread at [1]. > > I could not find an appropriate spot to place these comments, so I placed > them on bttextcmp() function, The only other place that I could see we can > place these comments is in the file src/backend/optimizer/README, because > there is some consideration given to leakproof functions in optimizer docs. > But these comments seem quite out of place in optimizer docs. It doesn't seem particularly likely that someone who is thinking about changing this in the future would notice the comment in the place where you propose to put it, nor that they would read the optimizer README. Furthermore, I don't know that everyone agrees with Tom about this. I do agree that it's more important to mark relational operators leakproof than other things, and I also agree that conservatism is warranted. But that does not mean that someone could not make a compelling argument for marking other functions leakproof. I think we will be better off leaving this alone. -- Robert Haas EDB: http://www.enterprisedb.com
