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 Mon, May 13, 2019 at 12:51 PM Robert Haas <robertmhaas@gmail.com> wrote:

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.

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.

On Wed, May 8, 2019 at 5:05 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:

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.

The topic of consistency for comment style for all wrappers seems got lost
with other discussion, would like to seek opinion on the same.

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

On Thu, May 23, 2019 at 4:32 PM Andres Freund <andres@anarazel.de> wrote:

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!

Thanks a lot Andres. With pg_intend run before the patch on master, I can
imagine possibly generated additional work for you on this.