Thread: renaming ExecStoreWhateverTuple
Hi, While reviewing some zheap code last week, I noticed that the naming of the ExecStore*Tuple() functions seems a bit unfortunate. One reason is that we are now using slots in all kinds of places other than the executor, so that the "Exec" prefix seems a bit out of place. However, you could argue that this is OK, on the grounds that the functions are defined in the executor. What I noticed, though, is that every new table AM is probably going to need its own type of slot, and it's natural to define those slot support functions in the AM code rather than having every AM shove whatever it's got into execTuples.c. But if you do that, then you end up with a function with a name like ExecStoreZHeapTuple() which isn't in src/backend/executor, and which furthermore will never be called from the executor, because the executor only knows about a short, hard-coded list of built-in slot types, and ZHeapTuples are not one of them. That function will, rather, only be called from the zheap code. And having a function whose name starts with Exec... that is neither defined in the executor nor used in the executor seems wrong. So I think we should rename these functions before we get too used to the new names. There is a significant advantage in doing that for v12 because people are already going to have to adjust third-party code to compensate for the fact that we no longer have an ExecStoreTuple() function any more. I'm not sure exactly what names would be better. Perhaps just change the "Exec" prefix to "Slot", e.g. SlotStoreHeapTuple(). Or maybe put InSlot at the end, like StoreHeapTupleInSlot(). Or just take those four words - slot, heap, tuple, and store - and put them in any order you like. TupleSlotStoreHeap? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I agree about taking out the "Exec" part of the name. On 2019-Mar-25, Robert Haas wrote: > I'm not sure exactly what names would be better. Perhaps just change > the "Exec" prefix to "Slot", e.g. SlotStoreHeapTuple(). Or maybe put > InSlot at the end, like StoreHeapTupleInSlot(). Or just take those > four words - slot, heap, tuple, and store - and put them in any order > you like. TupleSlotStoreHeap? HeapStoreTupleInSlot? (I wonder why not "Put" instead of "Store".) Should we keep ExecStoreTuple as a compatibility macro for third party code? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Mar 25, 2019 at 11:27 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Should we keep ExecStoreTuple as a compatibility macro for third party > code? I think that might be rather dangerous, actually, because slots now have a type, which they didn't before. You have to use the correct function for the kind of slot you've got. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2019-Mar-25, Robert Haas wrote: > On Mon, Mar 25, 2019 at 11:27 AM Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > Should we keep ExecStoreTuple as a compatibility macro for third party > > code? > > I think that might be rather dangerous, actually, because slots now > have a type, which they didn't before. You have to use the correct > function for the kind of slot you've got. Ah, right. I was thinking of assuming that the un-updated third-party code would always have a slot of type heap, but of course that's not guaranteed. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-03-25 11:20:13 -0400, Robert Haas wrote: > While reviewing some zheap code last week, I noticed that the naming > of the ExecStore*Tuple() functions seems a bit unfortunate. One > reason is that we are now using slots in all kinds of places other > than the executor, so that the "Exec" prefix seems a bit out of place. > However, you could argue that this is OK, on the grounds that the > functions are defined in the executor. What I noticed, though, is > that every new table AM is probably going to need its own type of > slot, and it's natural to define those slot support functions in the > AM code rather than having every AM shove whatever it's got into > execTuples.c. Yea, it's not accurate. Nor was it before this change though - there's e.g. plenty executor code that used slots long before v12. > But if you do that, then you end up with a function with a name like > ExecStoreZHeapTuple() which isn't in src/backend/executor, and which > furthermore will never be called from the executor, because the > executor only knows about a short, hard-coded list of built-in slot > types, and ZHeapTuples are not one of them. That function will, > rather, only be called from the zheap code. And having a function > whose name starts with Exec... that is neither defined in the executor > nor used in the executor seems wrong. Yea. I feel if we go there we probably should also rename ExecClearTuple, ExecMaterializeSlot, ExecCopySlotHeapTuple, ExecCopySlotMinimalTuple, ExecCopySlot. Although there we probably can add a compat wrapper. > So I think we should rename these functions before we get too used to > the new names. There is a significant advantage in doing that for v12 > because people are already going to have to adjust third-party code to > compensate for the fact that we no longer have an ExecStoreTuple() > function any more. Indeed. > I'm not sure exactly what names would be better. Perhaps just change > the "Exec" prefix to "Slot", e.g. SlotStoreHeapTuple(). Or maybe put > InSlot at the end, like StoreHeapTupleInSlot(). Or just take those > four words - slot, heap, tuple, and store - and put them in any order > you like. TupleSlotStoreHeap? I think I might go for slot_store_heap_tuple etc, given that a lot of those accessors have been slot_ for about ever. What do you think? Greetings, Andres Freund
On Mon, Mar 25, 2019 at 11:57 AM Andres Freund <andres@anarazel.de> wrote: > I think I might go for slot_store_heap_tuple etc, given that a lot of > those accessors have been slot_ for about ever. What do you think? I don't have a super-strong feeling about it, although changing the case convention might confuse a few people. Maybe we don't really need the word "tuple". Like we could just make it slot_store_heap() or SlotStoreHeap(). A slot can only store a tuple, after all. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 2019-03-25 12:05:52 -0400, Robert Haas wrote: > On Mon, Mar 25, 2019 at 11:57 AM Andres Freund <andres@anarazel.de> wrote: > > I think I might go for slot_store_heap_tuple etc, given that a lot of > > those accessors have been slot_ for about ever. What do you think? > > I don't have a super-strong feeling about it, although changing the > case convention might confuse a few people. Right, but is that going to be more people that are going to be confused by the difference between the already existing slot_ functions and the existing camel-cased stuff? Greetings, Andres Freund
Robert Haas <robertmhaas@gmail.com> writes:
> Maybe we don't really need the word "tuple". Like we could just make
> it slot_store_heap() or SlotStoreHeap(). A slot can only store a
> tuple, after all.
I don't think it's wise to think of these things as just "slots";
that name is way too generic. They are "tuple slots", and so that
word has to stay in the relevant function names.
regards, tom lane
Hi,
On 2019-03-25 12:33:38 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > Maybe we don't really need the word "tuple". Like we could just make
> > it slot_store_heap() or SlotStoreHeap(). A slot can only store a
> > tuple, after all.
>
> I don't think it's wise to think of these things as just "slots";
> that name is way too generic. They are "tuple slots", and so that
> word has to stay in the relevant function names.
Hm. But we already have slot_{getsomeattrs, getallattrs, attisnull,
getattr, getsysattr}. But perhaps the att in there is enough addiitional
information?
Although now I'm looking at consistency annoyed at slot_attisnull (no r)
and slot_getattr (r) ;)
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2019-03-25 12:33:38 -0400, Tom Lane wrote:
>> I don't think it's wise to think of these things as just "slots";
>> that name is way too generic. They are "tuple slots", and so that
>> word has to stay in the relevant function names.
> Hm. But we already have slot_{getsomeattrs, getallattrs, attisnull,
> getattr, getsysattr}. But perhaps the att in there is enough addiitional
> information?
I don't claim to be entirely innocent in this matter ;-)
If we're going to rename stuff in this area without concern for avoiding
inessential code churn, then those are valid targets as well.
BTW, maybe it's worth drawing a naming distinction between
slot-type-specific and slot-type-independent functions?
(I assume there are still some of the latter.)
regards, tom lane
Hi,
On 2019-03-25 12:45:36 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-03-25 12:33:38 -0400, Tom Lane wrote:
> >> I don't think it's wise to think of these things as just "slots";
> >> that name is way too generic. They are "tuple slots", and so that
> >> word has to stay in the relevant function names.
>
> > Hm. But we already have slot_{getsomeattrs, getallattrs, attisnull,
> > getattr, getsysattr}. But perhaps the att in there is enough addiitional
> > information?
>
> I don't claim to be entirely innocent in this matter ;-)
>
> If we're going to rename stuff in this area without concern for avoiding
> inessential code churn, then those are valid targets as well.
FWIW, I don't have much of a problem with current slot_ names.
> BTW, maybe it's worth drawing a naming distinction between
> slot-type-specific and slot-type-independent functions?
> (I assume there are still some of the latter.)
Hm, I guess that depends on what you classify as type independent. Is
something like slot_getattr type independent, even though it internally
calls slot_getsomeattrs, which'll call a callback if additional
deforming is necessary?
I'd say, if you exclude functions like that, ExecStoreVirtualTuple(),
ExecStoreAllNullTuple(), slot_getmissingattrs(), ExecSetSlotDescriptor()
are probably the only ones that have no awareness of the type of a
slot.
I'm not sure it matters that much however? Unless you store something in
a slot, code normally shouldn't have to care what type a slot is.
Greetings,
Andres Freund
On Mon, Mar 25, 2019 at 12:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > Maybe we don't really need the word "tuple". Like we could just make > > it slot_store_heap() or SlotStoreHeap(). A slot can only store a > > tuple, after all. > > I don't think it's wise to think of these things as just "slots"; > that name is way too generic. They are "tuple slots", and so that > word has to stay in the relevant function names. I suppose there is some potential for confusion with things like logical replication slots, but I think that these are the most widely-used type of slot in the backend, so it's not entirely crazy to think that they deserve a bit of special consideration. I'm not violently opposed to using four words instead of three (slot_store_heap_tuple vs. slot_store_heap) but to really spell out the operation in full you'd need to say something like HeapTupleTableSlotStoreHeapTuple, and I think that's pretty unwieldy for what's likely to end up being a very common programming idiom. It's not crazy that we type 'cd' to change directories rather than 'chdir' or 'change_directory'. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company