A structure has changed but comment modifications maybe missed

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：

https://github.com/postgres/postgres/blob/master/src/include/executor/tuptable.h
src/include/executor/tuptable.h：71
src/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

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

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

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