Remove all "INTERFACE ROUTINES" style comments

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Remove all "INTERFACE ROUTINES" style comments

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

Re: Remove all "INTERFACE ROUTINES" style comments

Thomas Munro-3
On Fri, Jan 11, 2019 at 12:58 PM Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Remove all "INTERFACE ROUTINES" style comments

Tom Lane-2
Thomas Munro <[hidden email]> writes:
> On Fri, Jan 11, 2019 at 12:58 PM Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Remove all "INTERFACE ROUTINES" style comments

Robert Haas
In reply to this post by Thomas Munro-3
On Thu, Jan 10, 2019 at 7:05 PM Thomas Munro
<[hidden email]> wrote:
> +1

+1

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: Remove all "INTERFACE ROUTINES" style comments

Andres Freund
In reply to this post by Andres Freund
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

0001-Remove-most-INTERFACE-ROUTINES-comments.patch (60K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Remove all "INTERFACE ROUTINES" style comments

Michael Paquier-2
In reply to this post by Robert Haas
On Fri, Jan 11, 2019 at 12:02:22PM -0500, Robert Haas wrote:
> On Thu, Jan 10, 2019 at 7:05 PM Thomas Munro
> <[hidden email]> wrote:
> > +1
>
> +1

+1.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Remove all "INTERFACE ROUTINES" style comments

Heikki Linnakangas
In reply to this post by Andres Freund
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