Thread: Remove all "INTERFACE ROUTINES" style comments
Hi, A number of postgres files have sections like heapam's * INTERFACE ROUTINES * relation_open - open any relation by relation OID * relation_openrv - open any relation specified by a RangeVar * relation_close - close any relation * heap_open - open a heap relation by relation OID * heap_openrv - open a heap relation specified by a RangeVar * heap_close - (now just a macro for relation_close) * heap_beginscan - begin relation scan * heap_rescan - restart a relation scan * heap_endscan - end relation scan * heap_getnext - retrieve next tuple in scan * heap_fetch - retrieve tuple with given tid * heap_insert - insert tuple into a relation * heap_multi_insert - insert multiple tuples into a relation * heap_delete - delete a tuple from a relation * heap_update - replace a tuple in a relation with another tuple * heap_sync - sync heap, for when no WAL has been written They're often out-of-date, and I personally never found them to be useful. A few people, including yours truly, have been removing a few here and there when overhauling a subsystem to avoid having to update and then adjust them. I think it might be a good idea to just do that for all at once. Having to consider separately committing a removal, updating them without fixing preexisting issues, or just leaving them outdated on a regular basis imo is a usless distraction. Comments? Greetings, Andres Freund
On Fri, Jan 11, 2019 at 12:58 PM Andres Freund <andres@anarazel.de> wrote: > A number of postgres files have sections like heapam's > > * INTERFACE ROUTINES > * relation_open - open any relation by relation OID > * relation_openrv - open any relation specified by a RangeVar > * relation_close - close any relation > * heap_open - open a heap relation by relation OID > * heap_openrv - open a heap relation specified by a RangeVar > * heap_close - (now just a macro for relation_close) > * heap_beginscan - begin relation scan > * heap_rescan - restart a relation scan > * heap_endscan - end relation scan > * heap_getnext - retrieve next tuple in scan > * heap_fetch - retrieve tuple with given tid > * heap_insert - insert tuple into a relation > * heap_multi_insert - insert multiple tuples into a relation > * heap_delete - delete a tuple from a relation > * heap_update - replace a tuple in a relation with another tuple > * heap_sync - sync heap, for when no WAL has been written > > They're often out-of-date, and I personally never found them to be > useful. A few people, including yours truly, have been removing a few > here and there when overhauling a subsystem to avoid having to update > and then adjust them. > > I think it might be a good idea to just do that for all at once. Having > to consider separately committing a removal, updating them without > fixing preexisting issues, or just leaving them outdated on a regular > basis imo is a usless distraction. > > Comments? +1 -- Thomas Munro http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Fri, Jan 11, 2019 at 12:58 PM Andres Freund <andres@anarazel.de> wrote:
>> A number of postgres files have sections like heapam's
>> * INTERFACE ROUTINES
>>
>> They're often out-of-date, and I personally never found them to be
>> useful. A few people, including yours truly, have been removing a few
>> here and there when overhauling a subsystem to avoid having to update
>> and then adjust them.
>> I think it might be a good idea to just do that for all at once.
> +1
I agree we don't maintain them well, so it'd be better to remove them,
as long as we make sure any useful info gets transferred someplace else
(like per-function header comments).
regards, tom lane
On Thu, Jan 10, 2019 at 7:05 PM Thomas Munro <thomas.munro@enterprisedb.com> wrote: > +1 +1 -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 2019-01-10 15:58:41 -0800, Andres Freund wrote: > A number of postgres files have sections like heapam's > > * INTERFACE ROUTINES > ... > They're often out-of-date, and I personally never found them to be > useful. A few people, including yours truly, have been removing a few > here and there when overhauling a subsystem to avoid having to update > and then adjust them. > > I think it might be a good idea to just do that for all at once. Having > to consider separately committing a removal, updating them without > fixing preexisting issues, or just leaving them outdated on a regular > basis imo is a usless distraction. As the reaction was positive, here's a first draft of a commit removing them. A few comments: - I left two INTERFACE ROUTINES blocks intact, because they actually add somewhat useful information. Namely fd.c's, which actually seems useful, and predicate.c's about which I'm less sure. - I tried to move all comments about the routines in the INTERFACE section to the functions if they didn't have a roughly equivalent comment. Even if the comment wasn't that useful. Particularly just about all the function comments in executor/node*.c files are useless, but I thought that's something best to be cleaned up separately. - After removing the INTERFACE ROUTINES blocks a number of executor files had a separate comment block with just a NOTES section. I merged these with the file header comment blocks, and indented them to match. I think this is better, but I'm only like 60% convinced of that. Comments? I'll revisit this patch on Monday or so, make another pass through it, and push it then. Greetings, Andres Freund
Attachment
On Fri, Jan 11, 2019 at 12:02:22PM -0500, Robert Haas wrote: > On Thu, Jan 10, 2019 at 7:05 PM Thomas Munro > <thomas.munro@enterprisedb.com> wrote: > > +1 > > +1 +1. -- Michael
Attachment
On 12/01/2019 03:12, Andres Freund wrote: > On 2019-01-10 15:58:41 -0800, Andres Freund wrote: >> A number of postgres files have sections like heapam's >> >> * INTERFACE ROUTINES >> ... >> They're often out-of-date, and I personally never found them to be >> useful. A few people, including yours truly, have been removing a few >> here and there when overhauling a subsystem to avoid having to update >> and then adjust them. >> >> I think it might be a good idea to just do that for all at once. Having >> to consider separately committing a removal, updating them without >> fixing preexisting issues, or just leaving them outdated on a regular >> basis imo is a usless distraction. > > As the reaction was positive, here's a first draft of a commit removing > them. A few comments: > > - I left two INTERFACE ROUTINES blocks intact, because they actually add > somewhat useful information. Namely fd.c's, which actually seems > useful, and predicate.c's about which I'm less sure. > - I tried to move all comments about the routines in the INTERFACE > section to the functions if they didn't have a roughly equivalent > comment. Even if the comment wasn't that useful. Particularly just > about all the function comments in executor/node*.c files are useless, > but I thought that's something best to be cleaned up separately. > - After removing the INTERFACE ROUTINES blocks a number of executor > files had a separate comment block with just a NOTES section. I merged > these with the file header comment blocks, and indented them to > match. I think this is better, but I'm only like 60% convinced of > that. > > Comments? I'll revisit this patch on Monday or so, make another pass > through it, and push it then. I agree that just listing all the public functions in a source file is not useful. But listing the most important ones, perhaps with examples on how to use them together, or grouping functions when there are a lot of them, is useful. A high-level view of the public interface is especially useful for people who are browsing the code for the first time. The comments in execMain.c seemed like a nice overview to the interface. I'm not sure the comments on each function do quite the same thing. The grouping of the functions in pqcomm.c's seemed useful. Especially when some of the routines listed there are actually macros defined in libpq.h, so if someone just looks at the contents of pqcomm.c, he might not realize that there's more in libpq.h. The grouping in pqformat.c also seemd useful. In that vein, the comments in heapam.c could be re-structured, something like this: * Opening/closing relations * ------------------------- * * The relation_* functions work on any relation, not only heap * relations: * * relation_open - open any relation by relation OID * relation_openrv - open any relation specified by a RangeVar * relation_close - close any relation * * These are similar, but check that the relation is a Heap * relation: * * heap_open - open a heap relation by relation OID * heap_openrv - open a heap relation specified by a RangeVar * heap_close - (now just a macro for relation_close) * * Heap scans * ---------- * * Functions for doing a Sequential Scan on a heap table: * * heap_beginscan - begin relation scan * heap_rescan - restart a relation scan * heap_endscan - end relation scan * heap_getnext - retrieve next tuple in scan * * To retrieve a single heap tuple, by tid: * heap_fetch - retrieve tuple with given tid * * Updating a heap * --------------- * * heap_insert - insert tuple into a relation * heap_multi_insert - insert multiple tuples into a relation * heap_delete - delete a tuple from a relation * heap_update - replace a tuple in a relation with another tuple * heap_sync - sync heap, for when no WAL has been written - Heikki