Обсуждение: Inconsistency between table am callback and table function names

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.


>  tuple_fetch_row_version
>  tuple_get_latest_tid

Hm, I'd not object to adding a tuple_ to the wrapper.


>  tuple_insert
>  tuple_insert_speculative
>  tuple_delete
>  tuple_update
>  tuple_lock

That again is to keep the naming similar to the existing functions.



> Also, some of these table function names read little odd
> 
> table_relation_set_new_filenode
> table_relation_nontransactional_truncate
> table_relation_copy_data
> table_relation_copy_for_cluster
> table_relation_vacuum
> table_relation_estimate_size
> 
> Can we drop relation word from callback names and as a result from these
> function names as well? Just have callback names as set_new_filenode,
> copy_data, estimate_size?

I'm strongly against that. These all work on a full relation size,
rather than on individual tuples, and that seems worth pointing out.


> 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.


> 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...

Greetings,

Andres Freund

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.

Hi,

On 2019-05-08 17:05:07 -0700, Ashwin Agrawal wrote:
> On Wed, May 8, 2019 at 2:51 PM Andres Freund <andres@anarazel.de> wrote:
> > 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.

The changes necessary for tableam were already huge. Changing naming
schemes for functions that are used all over the backend (e.g. ~80 calls
to table_beginscan), and where there's other wrapper functions that also
widely used (237 calls to systable_beginscan) which didn't have to be
touched, at the same time would have made it even harder to review.

Greetings,

Andres Freund

On Thu, May 9, 2019 at 8:52 AM Andres Freund <andres@anarazel.de> wrote:
> The changes necessary for tableam were already huge. Changing naming
> schemes for functions that are used all over the backend (e.g. ~80 calls
> to table_beginscan), and where there's other wrapper functions that also
> widely used (237 calls to systable_beginscan) which didn't have to be
> touched, at the same time would have made it even harder to review.

If there are no objections to renaming now, as separate independent
patch, I am happy to do the same and send it across. Will rename to
make it consistent as mentioned at start of the thread leaving
table_relation_xxx() ones as is today.

Hi,

On 2019-05-10 10:43:44 -0700, Ashwin Agrawal wrote:
> On Thu, May 9, 2019 at 8:52 AM Andres Freund <andres@anarazel.de> wrote:
> > The changes necessary for tableam were already huge. Changing naming
> > schemes for functions that are used all over the backend (e.g. ~80 calls
> > to table_beginscan), and where there's other wrapper functions that also
> > widely used (237 calls to systable_beginscan) which didn't have to be
> > touched, at the same time would have made it even harder to review.
> 
> If there are no objections to renaming now, as separate independent
> patch, I am happy to do the same and send it across. Will rename to
> make it consistent as mentioned at start of the thread leaving
> table_relation_xxx() ones as is today.

What would you want to rename precisely? Don't think it's useful to
start sending patches before we agree on something concrete.  I'm not on
board with patching hundreds systable_beginscan calls (that'll break a
lot of external code, besides the churn of in-core code), nor with the
APIs around that having a diverging name scheme.

Greetings,

Andres Freund

On Fri, May 10, 2019 at 10:51 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-05-10 10:43:44 -0700, Ashwin Agrawal wrote:
> > On Thu, May 9, 2019 at 8:52 AM Andres Freund <andres@anarazel.de> wrote:
> > > The changes necessary for tableam were already huge. Changing naming
> > > schemes for functions that are used all over the backend (e.g. ~80 calls
> > > to table_beginscan), and where there's other wrapper functions that also
> > > widely used (237 calls to systable_beginscan) which didn't have to be
> > > touched, at the same time would have made it even harder to review.
> >
> > If there are no objections to renaming now, as separate independent
> > patch, I am happy to do the same and send it across. Will rename to
> > make it consistent as mentioned at start of the thread leaving
> > table_relation_xxx() ones as is today.
>
> What would you want to rename precisely? Don't think it's useful to
> start sending patches before we agree on something concrete.  I'm not on
> board with patching hundreds systable_beginscan calls (that'll break a
> lot of external code, besides the churn of in-core code), nor with the
> APIs around that having a diverging name scheme.

Meant to stick the question mark in that email, somehow missed. Yes
not planning to spend any time on it if objections. Here is the list
of renames I wish to perform.

Lets start with low hanging ones.

table_rescan -> table_scan_rescan
git grep table_rescan | wc -l
6

table_insert -> table_tuple_insert
git grep tuple_insert | wc -l
13

table_insert_speculative -> table_tuple_insert_speculative
git grep tuple_insert_speculative | wc -l
5

table_delete -> table_tuple_delete (table_delete reads incorrect as
not deleting the table)
git grep tuple_delete | wc -l
8

table_update -> table_tuple_update
git grep tuple_update | wc -l
5

table_lock_tuple -> table_tuple_lock
git grep tuple_lock | wc -l
26


Below two you already mentioned no objections to rename
table_fetch_row_version -> table_tuple_fetch_row_version
table_get_latest_tid -> table_tuple_get_latest_tid


Now, table_beginscan and table_endscan are the ones which are
wide-spread. Desire seems we should keep it consistent with
systable_beginscan. Understand the constraints and churn aspect, given
that diverging naming scheme is unavoidable. Why not leave
systable_beginscan as it is and only rename table_beginscan and its
counterparts table_beginscan_xxx() atleast?

Index interfaces and table interfaces have some diverged naming scheme
already like index_getnext_slot and table_scan_getnextslot. Not
proposing to change them. But at least reducing wherever possible
sooner would be helpful.

Hi,

On 2019-05-10 12:43:06 -0700, Ashwin Agrawal wrote:
> On Fri, May 10, 2019 at 10:51 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2019-05-10 10:43:44 -0700, Ashwin Agrawal wrote:
> > > On Thu, May 9, 2019 at 8:52 AM Andres Freund <andres@anarazel.de> wrote:
> > > > The changes necessary for tableam were already huge. Changing naming
> > > > schemes for functions that are used all over the backend (e.g. ~80 calls
> > > > to table_beginscan), and where there's other wrapper functions that also
> > > > widely used (237 calls to systable_beginscan) which didn't have to be
> > > > touched, at the same time would have made it even harder to review.
> > >
> > > If there are no objections to renaming now, as separate independent
> > > patch, I am happy to do the same and send it across. Will rename to
> > > make it consistent as mentioned at start of the thread leaving
> > > table_relation_xxx() ones as is today.
> >
> > What would you want to rename precisely? Don't think it's useful to
> > start sending patches before we agree on something concrete.  I'm not on
> > board with patching hundreds systable_beginscan calls (that'll break a
> > lot of external code, besides the churn of in-core code), nor with the
> > APIs around that having a diverging name scheme.
> 
> Meant to stick the question mark in that email, somehow missed. Yes
> not planning to spend any time on it if objections. Here is the list
> of renames I wish to perform.
> 
> Lets start with low hanging ones.
> 
> table_rescan -> table_scan_rescan
> git grep table_rescan | wc -l
> 6
> 
> table_insert -> table_tuple_insert
> git grep tuple_insert | wc -l
> 13
> 
> table_insert_speculative -> table_tuple_insert_speculative
> git grep tuple_insert_speculative | wc -l
> 5
> 
> table_delete -> table_tuple_delete (table_delete reads incorrect as
> not deleting the table)
> git grep tuple_delete | wc -l
> 8
> 
> table_update -> table_tuple_update
> git grep tuple_update | wc -l
> 5
> 
> table_lock_tuple -> table_tuple_lock
> git grep tuple_lock | wc -l
> 26
> 
> 
> Below two you already mentioned no objections to rename
> table_fetch_row_version -> table_tuple_fetch_row_version
> table_get_latest_tid -> table_tuple_get_latest_tid
> 
> 
> Now, table_beginscan and table_endscan are the ones which are
> wide-spread. Desire seems we should keep it consistent with
> systable_beginscan. Understand the constraints and churn aspect, given
> that diverging naming scheme is unavoidable. Why not leave
> systable_beginscan as it is and only rename table_beginscan and its
> counterparts table_beginscan_xxx() atleast?
> 
> Index interfaces and table interfaces have some diverged naming scheme
> already like index_getnext_slot and table_scan_getnextslot. Not
> proposing to change them. But at least reducing wherever possible
> sooner would be helpful.

My personal opinion is that this is more churn than I think is useful to
tackle after feature freeze, with not sufficient benefits.  If others
chime in, voting to do this, I'm OK with doing that, but otherwise I
think there's more important stuff to do.

Greetings,

Andres Freund

On 2019-May-10, Andres Freund wrote:

> My personal opinion is that this is more churn than I think is useful to
> tackle after feature freeze, with not sufficient benefits.  If others
> chime in, voting to do this, I'm OK with doing that, but otherwise I
> think there's more important stuff to do.

One issue is that if we don't change things now, we can never change it
afterwards, so we should make some effort to ensure that naming is
sensible.  And we already changed the names of the whole interface.

I'm not voting to accept all of Ashwin's proposals right away, only to
have the names considered.

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

Hi,

On 2019-05-10 16:18:32 -0400, Alvaro Herrera wrote:
> On 2019-May-10, Andres Freund wrote:
> 
> > My personal opinion is that this is more churn than I think is useful to
> > tackle after feature freeze, with not sufficient benefits.  If others
> > chime in, voting to do this, I'm OK with doing that, but otherwise I
> > think there's more important stuff to do.
> 
> One issue is that if we don't change things now, we can never change it
> afterwards, so we should make some effort to ensure that naming is
> sensible.  And we already changed the names of the whole interface.

Well, the point is that there's symmetry with a lot of similar functions
that were *not* affected by the tableam changes. Cf. systable_beginscan
et al.  We could add wrappers etc to make it less painful, but then
there's no urgency either.

Greetings,

Andres Freund

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-May-10, Andres Freund wrote:
>> My personal opinion is that this is more churn than I think is useful to
>> tackle after feature freeze, with not sufficient benefits.  If others
>> chime in, voting to do this, I'm OK with doing that, but otherwise I
>> think there's more important stuff to do.

> One issue is that if we don't change things now, we can never change it
> afterwards, so we should make some effort to ensure that naming is
> sensible.  And we already changed the names of the whole interface.

Yeah.  I do not have an opinion on whether these changes are actually
improvements, but renaming right now is way less painful than it would
be to rename post-v12.  Let's try to get it right the first time,
especially with functions we already renamed in this cycle.

I do think that the "too much churn" argument has merit for places
that were *not* already changed in v12.  In particular I'd vote against
renaming the systable_xxx functions.

            regards, tom lane

On Fri, May 10, 2019 at 3:43 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> Meant to stick the question mark in that email, somehow missed. Yes
> not planning to spend any time on it if objections. Here is the list
> of renames I wish to perform.
>
> Lets start with low hanging ones.
>
> table_rescan -> table_scan_rescan
> table_insert -> table_tuple_insert
> table_insert_speculative -> table_tuple_insert_speculative
> table_delete -> table_tuple_delete
> table_update -> table_tuple_update
> table_lock_tuple -> table_tuple_lock
>
> Below two you already mentioned no objections to rename
> table_fetch_row_version -> table_tuple_fetch_row_version
> table_get_latest_tid -> table_tuple_get_latest_tid
>
> Now, table_beginscan and table_endscan are the ones which are
> wide-spread.

I vote to rename all the ones where the new name would contain "tuple"
and to leave the others alone.  i.e. leave table_beginscan,
table_endscan, and table_rescan as they are.  I think that there's
little benefit in standardizing table_rescan but not the other two,
and we seem to agree that tinkering with the other two gets into a
painful amount of churn.

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

On 2019-May-14, Ashwin Agrawal wrote:

> Thank you. Please find the patch to rename the agreed functions. It would
> be good to make all consistent instead of applying exception to three
> functions but seems no consensus on it. Given table_ api are new, we could
> modify them leaving systable_ ones as is, but as objections left that as is.

Hmm .. I'm surprised to find out that we only have one caller of
simple_table_insert, simple_table_delete, simple_table_update.  I'm not
sure I agree to the new names those got in the renaming patch, since
they're not really part of table AM proper ... do we really want to
offer those as separate entry points?  Why not just remove those routines?

Somewhat related: it's strange that CatalogTupleUpdate etc use
simple_heap_update instead of the tableam variants wrappers (I suppose
that's either because of bootstrapping concerns, or performance).  Would
it be too strange to have CatalogTupleInsert call heap_insert()
directly, and do away with simple_heap_insert?  (Equivalently for
update, delete).  I think those wrappers made perfect sense when we had
simple_heap_insert all around the place ... but now that we introduced
the CatalogTupleFoo wrappers, I don't think it does any longer.


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

Hi,

On 2019-05-14 16:27:47 -0400, Alvaro Herrera wrote:
> On 2019-May-14, Ashwin Agrawal wrote:
> 
> > Thank you. Please find the patch to rename the agreed functions. It would
> > be good to make all consistent instead of applying exception to three
> > functions but seems no consensus on it. Given table_ api are new, we could
> > modify them leaving systable_ ones as is, but as objections left that as is.
> 
> Hmm .. I'm surprised to find out that we only have one caller of
> simple_table_insert, simple_table_delete, simple_table_update.  I'm not
> sure I agree to the new names those got in the renaming patch, since
> they're not really part of table AM proper ... do we really want to
> offer those as separate entry points?  Why not just remove those routines?

I don't think it'd be better if execReplication.c has them inline - we'd
just have the exact same code inline. There's plenty extension out there
that use simple_heap_*, and without such wrappers, they'll all have to
copy the contents of simple_table_* too.  Also we'll probably want to
switch CatalogTuple* over to them at some point.


> Somewhat related: it's strange that CatalogTupleUpdate etc use
> simple_heap_update instead of the tableam variants wrappers (I suppose
> that's either because of bootstrapping concerns, or performance).

It's because the callers currently expect to work with heap tuples,
rather than slots. And changing that would have been a *LOT* of work (as
in: prohibitively much for v12).  I didn't want to create a slot for
each insertion, because that'd make them slower. But as Robert said on
IM (discussing something else), we already create a slot in most cases,
via CatalogIndexInsert().  Not sure if HOT updates and deletes are
common enough to make the slot creation in those cases measurable.


> Would it be too strange to have CatalogTupleInsert call heap_insert()
> directly, and do away with simple_heap_insert?  (Equivalently for
> update, delete).  I think those wrappers made perfect sense when we had
> simple_heap_insert all around the place ... but now that we introduced
> the CatalogTupleFoo wrappers, I don't think it does any longer.

I don't really see the advantage. Won't that just break a lot of code
that will continue to work otherwise, as long as you just use heap
tables? With the sole benefit of moving a bit of code from one place to
another?

Greetings,

Andres Freund

Hi,

On 2019-05-14 12:11:46 -0700, Ashwin Agrawal wrote:
> Thank you. Please find the patch to rename the agreed functions. It would
> be good to make all consistent instead of applying exception to three
> functions but seems no consensus on it. Given table_ api are new, we could
> modify them leaving systable_ ones as is, but as objections left that as is.

I've pushed a slightly modified version (rebase, some additional
newlines due to the longer function names) now. Thanks for the patch!

Greetings,

Andres Freund