Thread: A structure has changed but comment modifications maybe missed
Recently when I was reading the TupleTableSlot code, I noticed that the field "tts_tuple" mentioned in the comment has been removed in the higher version, but it still exists in the comment.
location：
src/include/executor/tuptable.h：71
src/include/executor/tuptable.h：271
On Sat, Mar 25, 2023 at 10:43 PM 甄明洋 <zhenmingyang@yeah.net> wrote:
Recently when I was reading the TupleTableSlot code, I noticed that the field "tts_tuple" mentioned in the comment has been removed in the higher version, but it still exists in the comment.location：src/include/executor/tuptable.h：71src/include/executor/tuptable.h：271
Good catch. This should be a minor oversight in 4da597ed, in which the
TupleTableSlot implementation has been splitted into different slot
types, and tts_tuple has been removed from TupleTableSlot struct.
Besides, at tuptable.h:71, I think TTS_SHOULDFREE should be
TTS_FLAG_SHOULDFREE.
Thanks
Richard
TupleTableSlot implementation has been splitted into different slot
types, and tts_tuple has been removed from TupleTableSlot struct.
Besides, at tuptable.h:71, I think TTS_SHOULDFREE should be
TTS_FLAG_SHOULDFREE.
Thanks
Richard
On Mon, Mar 27, 2023 at 10:05 AM Richard Guo <guofenglinux@gmail.com> wrote:
Besides, at tuptable.h:71, I think TTS_SHOULDFREE should be
TTS_FLAG_SHOULDFREE.
A further search shows several other places referring to TTS_SHOULDFREE,
which I think should be TTS_FLAG_SHOULDFREE.
tuptable.h:71
tuptable.h:82
execTuples.c:388
execTuples.c:557
Thanks
Richard
which I think should be TTS_FLAG_SHOULDFREE.
tuptable.h:71
tuptable.h:82
execTuples.c:388
execTuples.c:557
Thanks
Richard
On Mon, 27 Mar 2023 at 15:20, Richard Guo <guofenglinux@gmail.com> wrote: > A further search shows several other places referring to TTS_SHOULDFREE, > which I think should be TTS_FLAG_SHOULDFREE. > > tuptable.h:71 > tuptable.h:82 > execTuples.c:388 > execTuples.c:557 I think the attached is what you have in mind. Can you confirm? David
Attachment
David Rowley <dgrowleyml@gmail.com> writes:
> I think the attached is what you have in mind. Can you confirm?
tts_nvalid is still a thing, so I question your edit here:
--- a/src/include/executor/tuptable.h
+++ b/src/include/executor/tuptable.h
@@ -68,8 +68,7 @@
* A TupleTableSlot can also be "empty", indicated by flag TTS_FLAG_EMPTY set
* in tts_flags, holding no valid data. This is the only valid state for a
* freshly-created slot that has not yet had a tuple descriptor assigned to
- * it. In this state, TTS_SHOULDFREE should not be set in tts_flags, tts_tuple
- * must be NULL and tts_nvalid zero.
+ * it. In this state, TTS_FLAG_SHOULDFREE should not be set in tts_flags.
*
* The tupleDescriptor is simply referenced, not copied, by the TupleTableSlot
* code. The caller of ExecSetSlotDescriptor() is responsible for providing
Andres would really be the expert about these changes though.
regards, tom lane
On Thu, 30 Mar 2023 at 15:02, Tom Lane <tgl@sss.pgh.pa.us> wrote: > tts_nvalid is still a thing, so I question your edit here: Yeah, that should be kept. See the adjusted patch attached. David
Attachment
Hi, On 2023-03-30 16:00:23 +1300, David Rowley wrote: > See the adjusted patch attached. Looks reasonable to me. We could try to reference TTS_SHOULDFREE(), without mentioning flags, instead - but I think your version is better. Greetings, Andres Freund
On Thu, Mar 30, 2023 at 11:07 AM Andres Freund <andres@anarazel.de> wrote:
On 2023-03-30 16:00:23 +1300, David Rowley wrote:
> See the adjusted patch attached.
Looks reasonable to me. We could try to reference TTS_SHOULDFREE(), without
mentioning flags, instead - but I think your version is better.
+1.
Thanks
Richard
Richard
On Thu, 30 Mar 2023 at 16:07, Andres Freund <andres@anarazel.de> wrote: > Looks reasonable to me. We could try to reference TTS_SHOULDFREE(), without > mentioning flags, instead - but I think your version is better. Thanks. Pushed. David