Обсуждение: 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
Вложения
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
Вложения
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