Re: Inconsistency between table am callback and table function names

On Wed, May 8, 2019 at 2:51 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-05-08 00:32:22 -0700, Ashwin Agrawal wrote:
> > The general theme for table function names seem to be
> > "table_<am_callback_name>". For example table_scan_getnextslot() and its
> > corresponding callback scan_getnextslot(). Most of the table functions and
> > callbacks follow mentioned convention except following ones
> >
> >  table_beginscan
> >  table_endscan
> >  table_rescan
> >  table_fetch_row_version
> >  table_get_latest_tid
> >  table_insert
> >  table_insert_speculative
> >  table_complete_speculative
> >  table_delete
> >  table_update
> >  table_lock_tuple
> >
> > the corresponding callback names for them are
> >
> >  scan_begin
> >  scan_end
> >  scan_rescan
>
> The mismatch here is just due of backward compat with the existing
> function names.

I am missing something here, would like to know more. table_ seem all
new fresh naming. Hence IMO having consistency with surrounding and
related code carries more weight as I don't know backward compat
serving what purpose. Heap function names can continue to call with
same old names for backward compat if required.


> > Also, a question about comments. Currently, redundant comments are written
> > above callback functions as also above table functions. They differ
> > sometimes a little bit on descriptions but majority content still being the
> > same. Should we have only one place finalized to have the comments to keep
> > them in sync and also know which one to refer to?
>
> Note that the non-differing comments usually just refer to the other
> place. And there's legitimate differences in most of the ones that are
> both at the callback and the external functions - since the audience of
> both are difference, that IMO makes sense.
>

Not having consistency is the main aspect I wish to bring to
attention. Like for some callback functions the comment is

    /* see table_insert() for reference about parameters */
    void        (*tuple_insert) (Relation rel, TupleTableSlot *slot,
                                 CommandId cid, int options,
                                 struct BulkInsertStateData *bistate);

    /* see table_insert_speculative() for reference about parameters
*/
    void        (*tuple_insert_speculative) (Relation rel,
                                             TupleTableSlot *slot,
                                             CommandId cid,
                                             int options,
                                             struct
BulkInsertStateData *bistate,
                                             uint32 specToken);

Whereas for some other callbacks the parameter explanation exist in
both the places. Seems we should be consistent.
I feel in long run becomes pain to keep them in sync as comments
evolve. Like for example

    /*
     * Estimate the size of shared memory needed for a parallel scan
of this
     * relation. The snapshot does not need to be accounted for.
     */
    Size        (*parallelscan_estimate) (Relation rel);

parallescan_estimate is not having snapshot argument passed to it, but
table_parallescan_estimate does. So, this way chances are high they
going out of sync and missing to modify in both the places. Agree
though on audience being different for both. So, seems going with the
refer XXX for parameters seems fine approach for all the callbacks and
then only specific things to flag out for the AM layer to be aware can
live above the callback function.

> > Plus, file name amapi.h now seems very broad, if possible should be renamed
> > to indexamapi.h or indexam.h to follow tableam.h. No idea what's our policy
> > around header file renames.
>
> We probably should rename it, but not in 12...

Okay good to know.