pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedA structure has changed but comment modifications maybe missed
9+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2023-03-30 902ecd3bd4 Fix outdated comments regarding TupleTableSlots
* A structure has changed but comment modifications maybe missed
@ 2023-03-25 14:43 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: 甄明洋 @ 2023-03-25 14:43 UTC (permalink / raw)
To: pgsql-bugs
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: A structure has changed but comment modifications maybe missed
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
@ 2023-03-27 02:05 ` Richard Guo <guofenglinux@gmail.com>
2023-03-27 02:19 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Richard Guo @ 2023-03-27 02:05 UTC (permalink / raw)
To: 甄明洋 <zhenmingyang@yeah.net>; +Cc: pgsql-bugs
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: A structure has changed but comment modifications maybe missed
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
@ 2023-03-27 02:19 ` Richard Guo <guofenglinux@gmail.com>
2023-03-30 01:55 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Richard Guo @ 2023-03-27 02:19 UTC (permalink / raw)
To: 甄明洋 <zhenmingyang@yeah.net>; +Cc: pgsql-bugs
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: A structure has changed but comment modifications maybe missed
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-27 02:19 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
@ 2023-03-30 01:55 ` David Rowley <dgrowleyml@gmail.com>
2023-03-30 02:02 ` Re: A structure has changed but comment modifications maybe missed Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 9+ messages in thread
From: David Rowley @ 2023-03-30 01:55 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: 甄明洋 <zhenmingyang@yeah.net>; pgsql-bugs
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
Attachments:
fix_outdated_comments_regarding_tupletableslots.patch (application/octet-stream, 3.0K) download
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 9f18d46081..fda20ec318 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -385,8 +385,8 @@ tts_heap_materialize(TupleTableSlot *slot)
{
/*
* The tuple contained in this slot is not allocated in the memory
- * context of the given slot (else it would have TTS_SHOULDFREE set).
- * Copy the tuple into the given slot's memory context.
+ * context of the given slot (else it would have TTS_FLAG_SHOULDFREE
+ * set). Copy the tuple into the given slot's memory context.
*/
hslot->tuple = heap_copytuple(hslot->tuple);
}
@@ -554,8 +554,9 @@ tts_minimal_materialize(TupleTableSlot *slot)
{
/*
* The minimal tuple contained in this slot is not allocated in the
- * memory context of the given slot (else it would have TTS_SHOULDFREE
- * set). Copy the minimal tuple into the given slot's memory context.
+ * memory context of the given slot (else it would have
+ * TTS_FLAG_SHOULDFREE set). Copy the minimal tuple into the given
+ * slot's memory context.
*/
mslot->mintuple = heap_copy_minimal_tuple(mslot->mintuple);
}
diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
index c61734a15d..8a6c62fbae 100644
--- 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
@@ -79,8 +78,8 @@
* mechanism to do more. However, the slot will increment the tupdesc
* reference count if a reference-counted tupdesc is supplied.)
*
- * When TTS_SHOULDFREE is set in tts_flags, the physical tuple is "owned" by
- * the slot and should be freed when the slot's reference to the tuple is
+ * When TTS_FLAG_SHOULDFREE is set in tts_flags, the physical tuple is "owned"
+ * by the slot and should be freed when the slot's reference to the tuple is
* dropped.
*
* tts_values/tts_isnull are allocated either when the slot is created (when
@@ -268,7 +267,7 @@ typedef struct BufferHeapTupleTableSlot
* If buffer is not InvalidBuffer, then the slot is holding a pin on the
* indicated buffer page; drop the pin when we release the slot's
* reference to that buffer. (TTS_FLAG_SHOULDFREE should not be set in
- * such a case, since presumably tts_tuple is pointing into the buffer.)
+ * such a case, since presumably base.tuple is pointing into the buffer.)
*/
Buffer buffer; /* tuple's buffer, or InvalidBuffer */
} BufferHeapTupleTableSlot;
* Re: A structure has changed but comment modifications maybe missed
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-27 02:19 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-30 01:55 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
@ 2023-03-30 02:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 03:00 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Tom Lane @ 2023-03-30 02:02 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; Richard Guo <guofenglinux@gmail.com>; 甄明洋 <zhenmingyang@yeah.net>; pgsql-bugs
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:
inline patch 1: src/include/executor/tuptable.h (+1/-2 lines) download inline-1.patch
--- 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
* Re: A structure has changed but comment modifications maybe missed
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-27 02:19 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-30 01:55 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
2023-03-30 02:02 ` Re: A structure has changed but comment modifications maybe missed Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-03-30 03:00 ` David Rowley <dgrowleyml@gmail.com>
2023-03-30 03:07 ` Re: A structure has changed but comment modifications maybe missed Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 9+ messages in thread
From: David Rowley @ 2023-03-30 03:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andres Freund <andres@anarazel.de>; Richard Guo <guofenglinux@gmail.com>; 甄明洋 <zhenmingyang@yeah.net>; pgsql-bugs
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
Attachments:
fix_outdated_comments_regarding_tupletableslots_v2.patch (application/octet-stream, 3.1K) download
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 9f18d46081..fda20ec318 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -385,8 +385,8 @@ tts_heap_materialize(TupleTableSlot *slot)
{
/*
* The tuple contained in this slot is not allocated in the memory
- * context of the given slot (else it would have TTS_SHOULDFREE set).
- * Copy the tuple into the given slot's memory context.
+ * context of the given slot (else it would have TTS_FLAG_SHOULDFREE
+ * set). Copy the tuple into the given slot's memory context.
*/
hslot->tuple = heap_copytuple(hslot->tuple);
}
@@ -554,8 +554,9 @@ tts_minimal_materialize(TupleTableSlot *slot)
{
/*
* The minimal tuple contained in this slot is not allocated in the
- * memory context of the given slot (else it would have TTS_SHOULDFREE
- * set). Copy the minimal tuple into the given slot's memory context.
+ * memory context of the given slot (else it would have
+ * TTS_FLAG_SHOULDFREE set). Copy the minimal tuple into the given
+ * slot's memory context.
*/
mslot->mintuple = heap_copy_minimal_tuple(mslot->mintuple);
}
diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
index c61734a15d..2e13ecc3ff 100644
--- a/src/include/executor/tuptable.h
+++ b/src/include/executor/tuptable.h
@@ -68,8 +68,8 @@
* 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 and
+ * tts_nvalid should be set to zero.
*
* The tupleDescriptor is simply referenced, not copied, by the TupleTableSlot
* code. The caller of ExecSetSlotDescriptor() is responsible for providing
@@ -79,8 +79,8 @@
* mechanism to do more. However, the slot will increment the tupdesc
* reference count if a reference-counted tupdesc is supplied.)
*
- * When TTS_SHOULDFREE is set in tts_flags, the physical tuple is "owned" by
- * the slot and should be freed when the slot's reference to the tuple is
+ * When TTS_FLAG_SHOULDFREE is set in tts_flags, the physical tuple is "owned"
+ * by the slot and should be freed when the slot's reference to the tuple is
* dropped.
*
* tts_values/tts_isnull are allocated either when the slot is created (when
@@ -268,7 +268,7 @@ typedef struct BufferHeapTupleTableSlot
* If buffer is not InvalidBuffer, then the slot is holding a pin on the
* indicated buffer page; drop the pin when we release the slot's
* reference to that buffer. (TTS_FLAG_SHOULDFREE should not be set in
- * such a case, since presumably tts_tuple is pointing into the buffer.)
+ * such a case, since presumably base.tuple is pointing into the buffer.)
*/
Buffer buffer; /* tuple's buffer, or InvalidBuffer */
} BufferHeapTupleTableSlot;
* Re: A structure has changed but comment modifications maybe missed
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-27 02:19 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-30 01:55 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
2023-03-30 02:02 ` Re: A structure has changed but comment modifications maybe missed Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 03:00 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
@ 2023-03-30 03:07 ` Andres Freund <andres@anarazel.de>
2023-03-30 03:33 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-30 03:37 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
0 siblings, 2 replies; 9+ messages in thread
From: Andres Freund @ 2023-03-30 03:07 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Richard Guo <guofenglinux@gmail.com>; 甄明洋 <zhenmingyang@yeah.net>; pgsql-bugs
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: A structure has changed but comment modifications maybe missed
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-27 02:19 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-30 01:55 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
2023-03-30 02:02 ` Re: A structure has changed but comment modifications maybe missed Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 03:00 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
2023-03-30 03:07 ` Re: A structure has changed but comment modifications maybe missed Andres Freund <andres@anarazel.de>
@ 2023-03-30 03:33 ` Richard Guo <guofenglinux@gmail.com>
1 sibling, 0 replies; 9+ messages in thread
From: Richard Guo @ 2023-03-30 03:33 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: David Rowley <dgrowleyml@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; 甄明洋 <zhenmingyang@yeah.net>; pgsql-bugs
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: A structure has changed but comment modifications maybe missed
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-27 02:19 ` Re: A structure has changed but comment modifications maybe missed Richard Guo <guofenglinux@gmail.com>
2023-03-30 01:55 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
2023-03-30 02:02 ` Re: A structure has changed but comment modifications maybe missed Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 03:00 ` Re: A structure has changed but comment modifications maybe missed David Rowley <dgrowleyml@gmail.com>
2023-03-30 03:07 ` Re: A structure has changed but comment modifications maybe missed Andres Freund <andres@anarazel.de>
@ 2023-03-30 03:37 ` David Rowley <dgrowleyml@gmail.com>
1 sibling, 0 replies; 9+ messages in thread
From: David Rowley @ 2023-03-30 03:37 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Richard Guo <guofenglinux@gmail.com>; 甄明洋 <zhenmingyang@yeah.net>; pgsql-bugs
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
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2023-03-30 03:37 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-03-25 14:43 A structure has changed but comment modifications maybe missed 甄明洋 <zhenmingyang@yeah.net>
2023-03-27 02:05 ` Richard Guo <guofenglinux@gmail.com>
2023-03-27 02:19 ` Richard Guo <guofenglinux@gmail.com>
2023-03-30 01:55 ` David Rowley <dgrowleyml@gmail.com>
2023-03-30 02:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-30 03:00 ` David Rowley <dgrowleyml@gmail.com>
2023-03-30 03:07 ` Andres Freund <andres@anarazel.de>
2023-03-30 03:33 ` Richard Guo <guofenglinux@gmail.com>
2023-03-30 03:37 ` David Rowley <dgrowleyml@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox