Обсуждение: What to name the current heap after pluggable storage / what torename?

Hi,

The current pluggable table storage patchset [1] introduces the ability
to specify the access method of a table (CREATE TABLE ... USING
"ident"). The patchset currently names the current storage method
(i.e. heapam.c et al) "heap" (whereas e.g. zheap's in named, drumroll,
zheap).

I'm concerned that naming it heap, and the corresponding functions
fitting with that name, will be confusing. There's a lot of functions
that have a heap_ prefix (or similar) that aren't really dependent on
how the storage works, but much more general infrastructure (consider
e.g. heap_create_with_catalog()).

One solution is to just live with the confusion, add a few header
comments to files like src/backend/catalog/heap.c, explaining that
they're more general than heapam.c (and in the patch heapam_handler.c,
which mostly dispatches to heapam.c).

Another approach would be to come up with a different, potentially
witty, name for the current postgres table access method. Then either
rename a few of the heapam.c et functions, or live with the slightly
more confusing names.

Another would be to be aggressive in renaming, and deconflict by
renaming functions like heap_create[_with_catalog] etc to sound more
accurate. I think that has some appeal, because a lot of those names
aren't describing their tasks particularly well.

Greetings,

Andres Freund

[1] https://postgr.es/m/20180703070645.wchpu5muyto5n647%40alap3.anarazel.de

On 19/12/2018 05:17, Andres Freund wrote:
> I'm concerned that naming it heap, and the corresponding functions
> fitting with that name, will be confusing. There's a lot of functions
> that have a heap_ prefix (or similar) that aren't really dependent on
> how the storage works, but much more general infrastructure (consider
> e.g. heap_create_with_catalog()).

I'm wondering where the choice of the name "heap" originally came from
and what it refers to.  In "The Design of Postgres"[0], it is said that
"All relations will be stored as heaps (as in [ASTR76]) with an optional
collection of secondary indexes."  But ASTR76[1] does not mention the
word "heap", so it doesn't appear to refer to any specific method or
algorithm.

The heap is clearly not the tree data structure.  Is it meant in the
sense of memory allocation, a place to store a large amount of stuff in
a mostly unorganized way?


[0]: http://db.cs.berkeley.edu/papers/ERL-M85-95.pdf
[1]: http://daslab.seas.harvard.edu/reading-group/papers/astrahan-1976.pdf


-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Dec 19, 2018 at 7:44 PM Arkhena <Arkhena@gmail.com> wrote:
>> I'm wondering where the choice of the name "heap" originally came from
>> and what it refers to.
>
> It seems to me that "heap" is an Oracle word (as explained here[1]).
>
> > By default, a table is organized as a heap, which means that the database places rows where they fit best rather
thanin a user-specified order.
 

No, it's more widely used than that, and we're using it with the
standard meaning AFAIK:


https://docs.microsoft.com/en-us/sql/relational-databases/indexes/heaps-tables-without-clustered-indexes?view=sql-server-2017
http://docs.actian.com/ingres/10.2/index.html#page/DatabaseAdmin/Heap_Storage_Structure.htm

It just means tuples stored in no particular order (as opposed to eg
btree tables, in systems that support those).

-- 
Thomas Munro
http://www.enterprisedb.com

On 19/12/2018 11:15, Thomas Munro wrote:
> It just means tuples stored in no particular order (as opposed to eg
> btree tables, in systems that support those).

So would the proposed pluggable storage API allow index-organized base
storage and other non-heap layouts?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2018-12-19 12:02:38 +0100, Peter Eisentraut wrote:
> On 19/12/2018 11:15, Thomas Munro wrote:
> > It just means tuples stored in no particular order (as opposed to eg
> > btree tables, in systems that support those).
> 
> So would the proposed pluggable storage API allow index-organized base
> storage and other non-heap layouts?

Well, that depends on what "non-heap layouts" you're thinking of.  I
think there'd be some further work needed to make efficient IOTs
possible, but the patchset gets us a long way to be able to do that in a
pluggable fashion.  Biggest problem would probably be extending the
existing index AMs, for secondary indexes, to point to a key wider than
a tid, without loosing too much efficiency.

Greetings,

Andres Freund

Hi,

On 2018-Dec-19, Andres Freund wrote:

> Well, that depends on what "non-heap layouts" you're thinking of.  I
> think there'd be some further work needed to make efficient IOTs
> possible, but the patchset gets us a long way to be able to do that in a
> pluggable fashion.  Biggest problem would probably be extending the
> existing index AMs, for secondary indexes, to point to a key wider than
> a tid, without loosing too much efficiency.

I think the important question is will we eventually get the option to
do "CREATE TABLE ... STORAGE indexorg" (or whatever name) rather than
are we already getting that feature, and I think the answer is clearly
yes, so we should keep using the word "heap" in the name as the primary
feature of the historical implementation.

The "zheap" name makes it clear that it is still a heap; the main
difference (if I understand correctly) is how does tuple
updating/deletion work.

The current heap implementation is for "non-overwriting storage
management", but that's a mouthful and acronyms based on
"non-overwriting" don't seem great ("noheap" seems a bit silly.  Maybe
"nowheap" is better?  How about "nosheap"?)

Maybe we can take the easy way and use something like "stdheap".

Or just "nheap".

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

On Wed, Dec 19, 2018 at 08:32:28AM -0300, Alvaro Herrera wrote:
> Hi,
> 
> On 2018-Dec-19, Andres Freund wrote:
> 
> > Well, that depends on what "non-heap layouts" you're thinking of.  I
> > think there'd be some further work needed to make efficient IOTs
> > possible, but the patchset gets us a long way to be able to do that in a
> > pluggable fashion.  Biggest problem would probably be extending the
> > existing index AMs, for secondary indexes, to point to a key wider than
> > a tid, without loosing too much efficiency.
> 
> I think the important question is will we eventually get the option to
> do "CREATE TABLE ... STORAGE indexorg" (or whatever name) rather than
> are we already getting that feature, and I think the answer is clearly
> yes, so we should keep using the word "heap" in the name as the primary
> feature of the historical implementation.
> 
> The "zheap" name makes it clear that it is still a heap; the main
> difference (if I understand correctly) is how does tuple
> updating/deletion work.
> 
> The current heap implementation is for "non-overwriting storage
> management", but that's a mouthful and acronyms based on
> "non-overwriting" don't seem great ("noheap" seems a bit silly.  Maybe
> "nowheap" is better?  How about "nosheap"?)
> 
> Maybe we can take the easy way and use something like "stdheap".
> 
> Or just "nheap".

oheap for "original?"

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Dec 18, 2018 at 11:17 PM Andres Freund <andres@anarazel.de> wrote:
> The current pluggable table storage patchset [1] introduces the ability
> to specify the access method of a table (CREATE TABLE ... USING
> "ident"). The patchset currently names the current storage method
> (i.e. heapam.c et al) "heap" (whereas e.g. zheap's in named, drumroll,
> zheap).

I vote for calling the current heap "heap" - i.e. what the patchset is
currently doing.  As others have already noted, that's a perfectly
good word for storing stuff in no particular order, and it's also a
term with a very long history. If we call it "oheap" or "pile" or
something based on a clever pun, then we'll just be making users learn
a new word for, as far as I can see, no real benefit.

> Another would be to be aggressive in renaming, and deconflict by
> renaming functions like heap_create[_with_catalog] etc to sound more
> accurate. I think that has some appeal, because a lot of those names
> aren't describing their tasks particularly well.

I like that option.

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

> On 19 Dec 2018, at 20:21, Robert Haas <robertmhaas@gmail.com> wrote:

> I vote for calling the current heap "heap" - i.e. what the patchset is
> currently doing.  As others have already noted, that's a perfectly
> good word for storing stuff in no particular order, and it's also a
> term with a very long history.

FWIW, +1

cheers ./daniel

Hi,

On 2018-12-19 14:21:29 -0500, Robert Haas wrote:
> On Tue, Dec 18, 2018 at 11:17 PM Andres Freund <andres@anarazel.de> wrote:
> > Another would be to be aggressive in renaming, and deconflict by
> > renaming functions like heap_create[_with_catalog] etc to sound more
> > accurate. I think that has some appeal, because a lot of those names
> > aren't describing their tasks particularly well.
>
> I like that option.

I'd like to start doing that by moving the functions in the following
heapam.h block elsewhere:

/* in heap/heapam.c */
extern Relation relation_open(Oid relationId, LOCKMODE lockmode);
extern Relation try_relation_open(Oid relationId, LOCKMODE lockmode);
extern Relation relation_openrv(const RangeVar *relation, LOCKMODE lockmode);
extern Relation relation_openrv_extended(const RangeVar *relation,
                         LOCKMODE lockmode, bool missing_ok);
extern void relation_close(Relation relation, LOCKMODE lockmode);

extern Relation heap_open(Oid relationId, LOCKMODE lockmode);
extern Relation heap_openrv(const RangeVar *relation, LOCKMODE lockmode);
extern Relation heap_openrv_extended(const RangeVar *relation,
                     LOCKMODE lockmode, bool missing_ok);

ISTM that the first block would best belong into new files like
access/rel[ation].h and access/common/rel[ation].h.  I think the second
set should be renamed to be table_open() (with backward compat
redirects, there's way way too many references) and should go into
access/table.h access/table/table.c alongside tableam.[ch], but I could
also see just putting them into relation.[ch].

Comments?

Greetings,

Andres Freund

On Thu, Jan 10, 2019 at 7:05 PM Andres Freund <andres@anarazel.de> wrote:
> ISTM that the first block would best belong into new files like
> access/rel[ation].h and access/common/rel[ation].h.

+1.

> I think the second
> set should be renamed to be table_open() (with backward compat
> redirects, there's way way too many references) and should go into
> access/table.h access/table/table.c alongside tableam.[ch],

Sounds reasonable.

> but I could
> also see just putting them into relation.[ch].

I would view that as a less-preferred alternative, but not crazy.

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

Hi,

On 2019-01-11 12:01:36 -0500, Robert Haas wrote:
> On Thu, Jan 10, 2019 at 7:05 PM Andres Freund <andres@anarazel.de> wrote:
> > ISTM that the first block would best belong into new files like
> > access/rel[ation].h and access/common/rel[ation].h.
> 
> +1.
> 
> > I think the second
> > set should be renamed to be table_open() (with backward compat
> > redirects, there's way way too many references) and should go into
> > access/table.h access/table/table.c alongside tableam.[ch],
> 
> Sounds reasonable.
> 
> > but I could
> > also see just putting them into relation.[ch].
> 
> I would view that as a less-preferred alternative, but not crazy.

Here's a set of patches. Not commit quality, but enough to discuss.


The first patch, the only really interesting one, splits out
relation_(open|openrv|openrv_extended|close) into access/relation.h and access/common/relation.c
and
heap_(open|openrv|openrv_extended|close) into access/table.h and access/table/table.c

It's worthwhile to note that there's another header named
nodes/relation.h. But there's also utils/rel.h, so I couldn't think of a
another good name.

I'm basically thinking that table.h, even in the post pluggable storage
world, should not contain lower level functionality like dispatching
into table-am (that'll reside in tableam.h). But e.g. a
simple_table_(insert|update|delete) could live there, as well as
potentially some other heap_ functionality strewn around the backend.

I made table.h not include relation.h, which means that a few files
might need both.  I'm not sure that's the right choice, but it seems
easier to extend that later if shows to be painful, than to do the
reverse.

I've left the following in table.h:
/*
 * heap_ used to be the prefix for these routines, and a lot of code will just
 * continue to work without adaptions after the introduction of pluggable
 * storage, therefore just map these names.
 */
#define heap_open(r, l)                    table_open(r, l)
#define heap_openrv(r, l)                table_openrv(r, l)
#define heap_openrv_extended(r, l, m)    table_openrv_extended(r, l, m)
#define heap_close(r, l)                table_close(r, l)

and I think we should leave that in there for the forseeable future.


Patches 0002 replaces includes of heapam.h with relation.h / table.h
where appropriate. 0003 renames all in-core references of
heap_(open|openrv|openrv_extended|close) with the table_ variant.


Does this seem basically sensible? Different ideas?


Greetings,

Andres Freund

On Tue, Jan 15, 2019 at 10:23 PM Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> access/relation.[c|h] name is fine. Or how about access/rel.[c|h],
> because nodes/relation.h is related to planner. utils/rel.h is some how
> related to relation caches.

Insofar as we can reasonably do so, I'd rather pick unique names for
header files.  I know that there's no law that rules out having both
nodes/relation.h and access/relation.h, or likewise utils/rel.h and
access/rel.h; the computer won't be confused.  But it might create
some confusion among human beings, so my vote is for avoiding that
sort of thing if we can.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 15, 2019 at 10:23 PM Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> access/relation.[c|h] name is fine. Or how about access/rel.[c|h],
>> because nodes/relation.h is related to planner. utils/rel.h is some how
>> related to relation caches.

> Insofar as we can reasonably do so, I'd rather pick unique names for
> header files.

+1

            regards, tom lane

On January 16, 2019 8:08:09 AM PST, Robert Haas <robertmhaas@gmail.com> wrote:
>On Tue, Jan 15, 2019 at 10:23 PM Haribabu Kommi
><kommi.haribabu@gmail.com> wrote:
>> access/relation.[c|h] name is fine. Or how about access/rel.[c|h],
>> because nodes/relation.h is related to planner. utils/rel.h is some
>how
>> related to relation caches.
>
>Insofar as we can reasonably do so, I'd rather pick unique names for
>header files.  I know that there's no law that rules out having both
>nodes/relation.h and access/relation.h, or likewise utils/rel.h and
>access/rel.h; the computer won't be confused.  But it might create
>some confusion among human beings, so my vote is for avoiding that
>sort of thing if we can.

I prefer that too - if the new name isn't worse enough to make it hard to remember. I'd welcome suggestions that don't
conflict...

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Hi,

On 2019-01-16 08:20:37 -0800, Andres Freund wrote:
> On January 16, 2019 8:08:09 AM PST, Robert Haas <robertmhaas@gmail.com> wrote:
> >On Tue, Jan 15, 2019 at 10:23 PM Haribabu Kommi
> ><kommi.haribabu@gmail.com> wrote:
> >> access/relation.[c|h] name is fine. Or how about access/rel.[c|h],
> >> because nodes/relation.h is related to planner. utils/rel.h is some
> >how
> >> related to relation caches.
> >
> >Insofar as we can reasonably do so, I'd rather pick unique names for
> >header files.  I know that there's no law that rules out having both
> >nodes/relation.h and access/relation.h, or likewise utils/rel.h and
> >access/rel.h; the computer won't be confused.  But it might create
> >some confusion among human beings, so my vote is for avoiding that
> >sort of thing if we can.
> 
> I prefer that too - if the new name isn't worse enough to make it hard
> to remember. I'd welcome suggestions that don't conflict...

Unless somebody comes up with a better suggestion I'm planning to press
ahead with this one. It's large enough to be a bit of a pain to maintain
over time...  I'm absolutely not wedded to access/relation.h, so I'm
happy with another good suggestion, or somebody revising it subsequently.

Greetings,

Andres Freund

On 2019-01-18 14:19:41 -0800, Andres Freund wrote:
> Hi,
> 
> On 2019-01-16 08:20:37 -0800, Andres Freund wrote:
> > On January 16, 2019 8:08:09 AM PST, Robert Haas <robertmhaas@gmail.com> wrote:
> > >On Tue, Jan 15, 2019 at 10:23 PM Haribabu Kommi
> > ><kommi.haribabu@gmail.com> wrote:
> > >> access/relation.[c|h] name is fine. Or how about access/rel.[c|h],
> > >> because nodes/relation.h is related to planner. utils/rel.h is some
> > >how
> > >> related to relation caches.
> > >
> > >Insofar as we can reasonably do so, I'd rather pick unique names for
> > >header files.  I know that there's no law that rules out having both
> > >nodes/relation.h and access/relation.h, or likewise utils/rel.h and
> > >access/rel.h; the computer won't be confused.  But it might create
> > >some confusion among human beings, so my vote is for avoiding that
> > >sort of thing if we can.
> > 
> > I prefer that too - if the new name isn't worse enough to make it hard
> > to remember. I'd welcome suggestions that don't conflict...
> 
> Unless somebody comes up with a better suggestion I'm planning to press
> ahead with this one. It's large enough to be a bit of a pain to maintain
> over time...  I'm absolutely not wedded to access/relation.h, so I'm
> happy with another good suggestion, or somebody revising it subsequently.

And pushed.  If somebody is interested in renaming/splitting nodes/relation.h, I
think that'd be good, but if not, it's also not terrible.

- Andres

On 2018-12-19 14:21:29 -0500, Robert Haas wrote:
> On Tue, Dec 18, 2018 at 11:17 PM Andres Freund <andres@anarazel.de> wrote:
> > The current pluggable table storage patchset [1] introduces the ability
> > to specify the access method of a table (CREATE TABLE ... USING
> > "ident"). The patchset currently names the current storage method
> > (i.e. heapam.c et al) "heap" (whereas e.g. zheap's in named, drumroll,
> > zheap).
> 
> I vote for calling the current heap "heap" - i.e. what the patchset is
> currently doing.  As others have already noted, that's a perfectly
> good word for storing stuff in no particular order, and it's also a
> term with a very long history. If we call it "oheap" or "pile" or
> something based on a clever pun, then we'll just be making users learn
> a new word for, as far as I can see, no real benefit.
> 
> > Another would be to be aggressive in renaming, and deconflict by
> > renaming functions like heap_create[_with_catalog] etc to sound more
> > accurate. I think that has some appeal, because a lot of those names
> > aren't describing their tasks particularly well.
> 
> I like that option.

In that vein, does anybody have an opinion about the naming of
a) HeapUpdateFailureData, which will be used for different AMs
b) HTSU_Result itself, which'll be the return parameter for
   update/delete via tableam
c) Naming of HTSU_Result members (like HeapTupleBeingUpdated)

I can see us doing several things:
1) Live with the old names, explain the naming as historical baggage
2) Replace names, but add typedefs / #defines for backward compatibility
3) Rename without backward compatibility

If we were to go with 2) or 3), does anybody want to make a case for
renaming the HTSU_Result members? They've been confusing people for a
long while...

In the patch it's currently:

typedef enum
{
    HeapTupleMayBeUpdated,        /* or deleted */
    HeapTupleInvisible,
    HeapTupleSelfUpdated,        /* or deleted */
    HeapTupleUpdated,
    HeapTupleDeleted,
    HeapTupleBeingUpdated,        /* or deleted */
    HeapTupleWouldBlock            /* can be returned by heap_tuple_lock */
} HTSU_Result;

Greetings,

Andres Freund

On Tue, Mar 12, 2019 at 8:39 PM Andres Freund <andres@anarazel.de> wrote:
> > I like that option.
>
> In that vein, does anybody have an opinion about the naming of
> a) HeapUpdateFailureData, which will be used for different AMs
> b) HTSU_Result itself, which'll be the return parameter for
>    update/delete via tableam
> c) Naming of HTSU_Result members (like HeapTupleBeingUpdated)
>
> I can see us doing several things:
> 1) Live with the old names, explain the naming as historical baggage
> 2) Replace names, but add typedefs / #defines for backward compatibility
> 3) Rename without backward compatibility

I think I have a mild preference for renaming HeapUpdateFailureData to
TableUpdateFailureData, but I'm less excited about renaming
HTSU_Result or its members.  I don't care if you want to do
s/HeapTuple/TableTuple/g and s/HTSU_Result/TTSU_Result/g though.

> If we were to go with 2) or 3), does anybody want to make a case for
> renaming the HTSU_Result members? They've been confusing people for a
> long while...
>
> In the patch it's currently:
>
> typedef enum
> {
>         HeapTupleMayBeUpdated,          /* or deleted */
>         HeapTupleInvisible,
>         HeapTupleSelfUpdated,           /* or deleted */
>         HeapTupleUpdated,
>         HeapTupleDeleted,
>         HeapTupleBeingUpdated,          /* or deleted */
>         HeapTupleWouldBlock                     /* can be returned by heap_tuple_lock */
> } HTSU_Result;

I think you're getting at the idea that HeapTupleMayBeUpdated really
means NoProblemsFound, but I would be inclined to leave that alone.
It's confusing, but the people who need to know what it means probably
have the current name figured out, and would have to learn what some
new name means.

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

Hi,

On 2019-03-13 08:29:47 -0400, Robert Haas wrote:
> On Tue, Mar 12, 2019 at 8:39 PM Andres Freund <andres@anarazel.de> wrote:
> > > I like that option.
> >
> > In that vein, does anybody have an opinion about the naming of
> > a) HeapUpdateFailureData, which will be used for different AMs
> > b) HTSU_Result itself, which'll be the return parameter for
> >    update/delete via tableam
> > c) Naming of HTSU_Result members (like HeapTupleBeingUpdated)
> >
> > I can see us doing several things:
> > 1) Live with the old names, explain the naming as historical baggage
> > 2) Replace names, but add typedefs / #defines for backward compatibility
> > 3) Rename without backward compatibility
> 
> I think I have a mild preference for renaming HeapUpdateFailureData to
> TableUpdateFailureData, but I'm less excited about renaming
> HTSU_Result or its members.  I don't care if you want to do
> s/HeapTuple/TableTuple/g and s/HTSU_Result/TTSU_Result/g though.

Anybody else got an opion on those? I personally don't have meaningful
feelings on this.  I'm mildly inclined to the renamings suggested
above.


> > If we were to go with 2) or 3), does anybody want to make a case for
> > renaming the HTSU_Result members? They've been confusing people for a
> > long while...
> >
> > In the patch it's currently:
> >
> > typedef enum
> > {
> >         HeapTupleMayBeUpdated,          /* or deleted */
> >         HeapTupleInvisible,
> >         HeapTupleSelfUpdated,           /* or deleted */
> >         HeapTupleUpdated,
> >         HeapTupleDeleted,
> >         HeapTupleBeingUpdated,          /* or deleted */
> >         HeapTupleWouldBlock                     /* can be returned by heap_tuple_lock */
> > } HTSU_Result;
> 
> I think you're getting at the idea that HeapTupleMayBeUpdated really
> means NoProblemsFound, but I would be inclined to leave that alone.
> It's confusing, but the people who need to know what it means probably
> have the current name figured out, and would have to learn what some
> new name means.

That, and that HeapTupleMayBeUpdated, HeapTupleBeingUpdated also can
mean that the tuple is actually being deleted, not updated.  The patch
currently adds HeapTupleDeleted (which is currently subsumed in
HeapTupleUpdated), to allow callsites to distinguish between those two
cases - but we don't need callsites to distinguish between
HeapTupleMayBeUpdated / Deleted (there's no meaningful difference imo),
and HeapTupleBeingUpdated / Deleted currently also isn't necessary,
although there's certainly a meaningful difference.

Greetings,

Andres Freund

On 2019-03-18 16:24:40 -0700, Andres Freund wrote:
> Hi,
> 
> On 2019-03-13 08:29:47 -0400, Robert Haas wrote:
> > On Tue, Mar 12, 2019 at 8:39 PM Andres Freund <andres@anarazel.de> wrote:
> > > > I like that option.
> > >
> > > In that vein, does anybody have an opinion about the naming of
> > > a) HeapUpdateFailureData, which will be used for different AMs
> > > b) HTSU_Result itself, which'll be the return parameter for
> > >    update/delete via tableam
> > > c) Naming of HTSU_Result members (like HeapTupleBeingUpdated)
> > >
> > > I can see us doing several things:
> > > 1) Live with the old names, explain the naming as historical baggage
> > > 2) Replace names, but add typedefs / #defines for backward compatibility
> > > 3) Rename without backward compatibility
> > 
> > I think I have a mild preference for renaming HeapUpdateFailureData to
> > TableUpdateFailureData, but I'm less excited about renaming
> > HTSU_Result or its members.  I don't care if you want to do
> > s/HeapTuple/TableTuple/g and s/HTSU_Result/TTSU_Result/g though.
> 
> Anybody else got an opion on those? I personally don't have meaningful
> feelings on this.  I'm mildly inclined to the renamings suggested
> above.

What I now have is:

/*
 * Result codes for table_{update,delete,lock}_tuple, and for visibility
 * routines inside table AMs.
 */
typedef enum TM_Result
{
    /* Signals that the action succeeded (i.e. update/delete performed) */
    TableTupleMayBeModified,

    /* The affected tuple wasn't visible to the relevant snapshot */
    TableTupleInvisible,

    /* The affected tuple was already modified by the calling backend */
    TableTupleSelfModified,

    /* The affected tuple was updated by another transaction */
    TableTupleUpdated,

    /* The affected tuple was deleted by another transaction */
    TableTupleDeleted,

    /*
     * The affected tuple is currently being modified by another session. This
     * will only be returned if (update/delete/lock)_tuple are instructed not
     * to wait.
     */
    TableTupleBeingModified,

    /* lock couldn't be acquired, action skipped. Only used by lock_tuple */
    TableTupleWouldBlock
} TM_Result;

I got rid of the SU part of the prefix, because SatisfiesUpdate isn't
actually exposed outside of the AMs.

I'm kinda wondering about replacing the TableTuple prefix with TableMod,
seems less confusing to me.  I'm also wondering about replacing
*MayBeModified with *OK.

Right now I have

typedef enum TM_Result HTSU_Result;

#define HeapTupleMayBeUpdated TableTupleMayBeModified
#define HeapTupleInvisible TableTupleInvisible
#define HeapTupleSelfUpdated TableTupleSelfModified
#define HeapTupleUpdated TableTupleUpdated
#define HeapTupleDeleted TableTupleDeleted
#define HeapTupleBeingUpdated TableTupleBeingModified
#define HeapTupleWouldBlock TableTupleWouldBlock

in heapam.h (whereas the above is in tableam.h), for backward
compat. But I'm not sure it's worth it.

Greetings,

Andres Freund

Hi,

On 2019-03-19 15:25:44 +1100, Haribabu Kommi wrote:
> On Tue, Mar 19, 2019 at 2:32 PM Andres Freund <andres@anarazel.de> wrote:
> > I'm kinda wondering about replacing the TableTuple prefix with TableMod,
> > seems less confusing to me.

> One more way, how about just TupleUpdated and etc. Removing of Table?
> The structure name also suggests as TM (IMO, it is TupleModication?)

Enum members are in the global namespace. I could go for a TM_ prefix
however.


>   I'm also wondering about replacing
> > *MayBeModified with *OK.

> How about TupleModified? I am fine with *OK also.

Hm, OK just seems nicer for lock_tuple, that's why I was wondering
about it, but I think my concern there is a bit pedantic.


> > Right now I have
> >
> > typedef enum TM_Result HTSU_Result;
> >
> > #define HeapTupleMayBeUpdated TableTupleMayBeModified
> > #define HeapTupleInvisible TableTupleInvisible
> > #define HeapTupleSelfUpdated TableTupleSelfModified
> > #define HeapTupleUpdated TableTupleUpdated
> > #define HeapTupleDeleted TableTupleDeleted
> > #define HeapTupleBeingUpdated TableTupleBeingModified
> > #define HeapTupleWouldBlock TableTupleWouldBlock
> >
> > in heapam.h (whereas the above is in tableam.h), for backward
> > compat. But I'm not sure it's worth it.
> >
> 
> These old macros are pretty much used in the internal code, and I doubt
> that any one depends directly on those macros. I vote for removal of
> these backward compatibility macros.

Cool.

Greetings,

Andres Freund