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
