Thread: Acceptable/Best formatting of callbacks (for pluggable storage)

Acceptable/Best formatting of callbacks (for pluggable storage)

From

Andres Freund

Date:

11 January 2019, 07:45:07

Hi,

The pluggable storage patchset has a large struct full of callbacks, and
a bunch of wrapper functions for calling those callbacks. While
starting to polish the patchset, I tried to make the formatting nice.

By default pgindent yields formatting like:

/*
 * API struct for a table AM.  Note this must be allocated in a
 * server-lifetime manner, typically as a static const struct, which then gets
 * returned by FormData_pg_am.amhandler.
 */
typedef struct TableAmRoutine
{
    NodeTag     type;

...
    void        (*relation_set_new_filenode) (Relation relation,
                                              char persistence,
                                              TransactionId *freezeXid,
                                              MultiXactId *minmulti);

...


static inline void
table_set_new_filenode(Relation rel, char persistence,
                       TransactionId *freezeXid, MultiXactId *minmulti)
{
    rel->rd_tableam->relation_set_new_filenode(rel, persistence,
                                               freezeXid, minmulti);
}

which isn't particularly pretty, especially because there's callbacks
with longer names than the example above.


Unfortunately pgindent prevents formatting the callbacks like:
    void        (*relation_set_new_filenode) (
        Relation relation,
        char persistence,
        TransactionId *freezeXid,
        MultiXactId *minmulti);

or something in that vein.  What however does work, is:

    void        (*relation_set_new_filenode)
                (Relation relation,
                 char persistence,
                 TransactionId *freezeXid,
                 MultiXactId *minmulti);

I.e. putting the opening ( of the parameter list into a separate line
yields somewhat usable formatting. This also has the advantage that the
arguments of all callbacks line up, making it a bit easier to scan.

Similarly, to reduce the indentation, especially for callbacks with long
names and/o with longer paramter names, we can do:

static inline void
table_set_new_filenode(Relation rel, char persistence,
                       TransactionId *freezeXid, MultiXactId *minmulti)
{
    rel->rd_tableam->relation_set_new_filenode
        (rel, persistence, freezeXid, minmulti);
}


So, putting the parameter list, both in use and declaration, entirely
into a new line yields decent formatting with pgindent. But it's kinda
weird.  I can't really come up with a better alternative, and after a
few minutes it looks pretty reasonable.

Comments? Better alternatives?

Greetings,

Andres Freund

Re: Acceptable/Best formatting of callbacks (for pluggable storage)

From

Tom Lane

Date:

11 January 2019, 17:42:19

Andres Freund <andres@anarazel.de> writes:
> The pluggable storage patchset has a large struct full of callbacks, and
> a bunch of wrapper functions for calling those callbacks. While
> starting to polish the patchset, I tried to make the formatting nice.
> ...
> So, putting the parameter list, both in use and declaration, entirely
> into a new line yields decent formatting with pgindent. But it's kinda
> weird.  I can't really come up with a better alternative, and after a
> few minutes it looks pretty reasonable.

> Comments? Better alternatives?

Use shorter method names?  This sounds like an ugly workaround for
a carpal-tunnel-syndrome-inducing design.

            regards, tom lane

Re: Acceptable/Best formatting of callbacks (for pluggable storage)

From

Robert Haas

Date:

11 January 2019, 18:32:03

On Thu, Jan 10, 2019 at 11:45 PM Andres Freund <andres@anarazel.de> wrote:
>     void        (*relation_set_new_filenode) (Relation relation,
>                                               char persistence,
>                                               TransactionId *freezeXid,
>                                               MultiXactId *minmulti);

Honestly, I don't see the problem with that, really.  But you could
also use the style that is used in fdwapi.h, where we have a typedef
for each callback first, and then the actual structure just declares a
function pointer of each time.  That saves a bit of horizontal space
and might look a little nicer.

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

Re: Acceptable/Best formatting of callbacks (for pluggable storage)

From

Robert Haas

Date:

11 January 2019, 18:33:48

On Fri, Jan 11, 2019 at 9:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Use shorter method names?  This sounds like an ugly workaround for
> a carpal-tunnel-syndrome-inducing design.

What would you suggest instead of something like
"relation_set_new_filenode"?  I agree that shorter names have some
merit, but it's not always easy to figure out how to shorten them
without making the result unclear.

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

Re: Acceptable/Best formatting of callbacks (for pluggable storage)

From

Andres Freund

Date:

11 January 2019, 20:56:42

Hi,

On 2019-01-11 10:32:03 -0500, Robert Haas wrote:
> On Thu, Jan 10, 2019 at 11:45 PM Andres Freund <andres@anarazel.de> wrote:
> >     void        (*relation_set_new_filenode) (Relation relation,
> >                                               char persistence,
> >                                               TransactionId *freezeXid,
> >                                               MultiXactId *minmulti);
> 
> Honestly, I don't see the problem with that, really.

It's just hard to read if there's a lot of callbacks defined, the more
accurate the name, the more deeply indented. Obviously that's always a
concern and thing to balance, but the added indentation due to the
whitespace, and the parens, * and whitespace between ) ( make it worse.


> But you could
> also use the style that is used in fdwapi.h, where we have a typedef
> for each callback first, and then the actual structure just declares a
> function pointer of each time.  That saves a bit of horizontal space
> and might look a little nicer.

It's what the patch did at first.  It doesn't save much space, because
the indentation due to the typedef at the start of the line is about as
much as defining in the struct adds, and we often add a _function
suffix.  It additionally adds a fair bit of mental overhead - there's
another set of names that one needs to keep track of, figuring out what
a callback means requires looking in an additional place.  I found that
removing that indirection made for a significantly more pleasant
experience working on the patchset.


Just as an example of why I think this isn't great:

typedef Size (*EstimateDSMForeignScan_function) (ForeignScanState *node,
                                                 ParallelContext *pcxt);
typedef void (*InitializeDSMForeignScan_function) (ForeignScanState *node,
                                                   ParallelContext *pcxt,
                                                   void *coordinate);
typedef void (*ReInitializeDSMForeignScan_function) (ForeignScanState *node,
                                                     ParallelContext *pcxt,
                                                     void *coordinate);
typedef void (*InitializeWorkerForeignScan_function) (ForeignScanState *node,
                                                      shm_toc *toc,
                                                      void *coordinate);
typedef void (*ShutdownForeignScan_function) (ForeignScanState *node);
typedef bool (*IsForeignScanParallelSafe_function) (PlannerInfo *root,
                                                    RelOptInfo *rel,
                                                    RangeTblEntry *rte);
typedef List *(*ReparameterizeForeignPathByChild_function) (PlannerInfo *root,
                                                            List *fdw_private,
                                                            RelOptInfo *child_rel);

that's a lot of indentation variability in a small amount of space - I
find it noticably slower to mentally parse due to that. Compare that
with:

typedef Size (*EstimateDSMForeignScan_function)
    (ForeignScanState *node,
     ParallelContext *pcxt);

typedef void (*InitializeDSMForeignScan_function)
    (ParallelContext *pcxt,
     void *coordinate);

typedef void (*ReInitializeDSMForeignScan_function)
    (ForeignScanState *node,
     ParallelContext *pcxt,
     void *coordinate);

typedef void (*InitializeWorkerForeignScan_function)
    (ForeignScanState *node,
     shm_toc *toc,
     void *coordinate);

typedef void (*ShutdownForeignScan_function)
    (ForeignScanState *node);

typedef bool (*IsForeignScanParallelSafe_function)
    (PlannerInfo *root,
     RelOptInfo *rel,
     RangeTblEntry *rte);

typedef List *(*ReparameterizeForeignPathByChild_function)
    (PlannerInfo *root,
     List *fdw_private,
     RelOptInfo *child_rel);

I find the second formatting considerably easier to read, albeit not for
the first few seconds.


Greetings,

Andres Freund

Re: Acceptable/Best formatting of callbacks (for pluggable storage)

From

Andres Freund

Date:

11 January 2019, 20:58:32

Hi,

On 2019-01-11 09:42:19 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > The pluggable storage patchset has a large struct full of callbacks, and
> > a bunch of wrapper functions for calling those callbacks. While
> > starting to polish the patchset, I tried to make the formatting nice.
> > ...
> > So, putting the parameter list, both in use and declaration, entirely
> > into a new line yields decent formatting with pgindent. But it's kinda
> > weird.  I can't really come up with a better alternative, and after a
> > few minutes it looks pretty reasonable.
> 
> > Comments? Better alternatives?
> 
> Use shorter method names?  This sounds like an ugly workaround for
> a carpal-tunnel-syndrome-inducing design.

I'm confused. What did I write about that has unreasonably long names?
And if you're referring to the wider design, that all seems fairly
fundamental to something needing callbacks - not exactly a first in
postgres.

Greetings,

Andres Freund

Re: Acceptable/Best formatting of callbacks (for pluggable storage)

From

Alvaro Herrera

Date:

17 January 2019, 18:05:50

On 2019-Jan-11, Andres Freund wrote:

> Just as an example of why I think this isn't great:

Hmm, to me, the first example is much better because of *vertical* space
-- I can have the whole API in one screenful.  In the other example, the
same number of functions take many more lines.  The fact that the
arguments are indented differently doesn't bother me.


> typedef Size (*EstimateDSMForeignScan_function) (ForeignScanState *node,
>                                                  ParallelContext *pcxt);
> typedef void (*InitializeDSMForeignScan_function) (ForeignScanState *node,
>                                                    ParallelContext *pcxt,
>                                                    void *coordinate);
> typedef void (*ReInitializeDSMForeignScan_function) (ForeignScanState *node,
>                                                      ParallelContext *pcxt,
>                                                      void *coordinate);
> typedef void (*InitializeWorkerForeignScan_function) (ForeignScanState *node,
>                                                       shm_toc *toc,
>                                                       void *coordinate);
> typedef void (*ShutdownForeignScan_function) (ForeignScanState *node);
> typedef bool (*IsForeignScanParallelSafe_function) (PlannerInfo *root,
>                                                     RelOptInfo *rel,
>                                                     RangeTblEntry *rte);
> typedef List *(*ReparameterizeForeignPathByChild_function) (PlannerInfo *root,
>                                                             List *fdw_private,
>                                                             RelOptInfo *child_rel);
> 
> that's a lot of indentation variability in a small amount of space - I
> find it noticably slower to mentally parse due to that. Compare that
> with:
> 
> typedef Size (*EstimateDSMForeignScan_function)
>     (ForeignScanState *node,
>      ParallelContext *pcxt);
> 
> typedef void (*InitializeDSMForeignScan_function)
>     (ParallelContext *pcxt,
>      void *coordinate);
> 
> typedef void (*ReInitializeDSMForeignScan_function)
>     (ForeignScanState *node,
>      ParallelContext *pcxt,
>      void *coordinate);
> 
> typedef void (*InitializeWorkerForeignScan_function)
>     (ForeignScanState *node,
>      shm_toc *toc,
>      void *coordinate);
> 
> typedef void (*ShutdownForeignScan_function)
>     (ForeignScanState *node);
> 
> typedef bool (*IsForeignScanParallelSafe_function)
>     (PlannerInfo *root,
>      RelOptInfo *rel,
>      RangeTblEntry *rte);
> 
> typedef List *(*ReparameterizeForeignPathByChild_function)
>     (PlannerInfo *root,
>      List *fdw_private,
>      RelOptInfo *child_rel);
> 
> I find the second formatting considerably easier to read, albeit not for
> the first few seconds.


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services