typedef struct WindowClause misleading comments
Hi.
Query jumbling considers the ORDER BY clause; therefore, the comment
below is inaccurate.
/
* The information relevant for the query jumbling is the partition clause
* type and its bounds.
*/
typedef struct WindowClause
{
NodeTag type;
/* window name (NULL in an OVER clause) */
char *name pg_node_attr(query_jumble_ignore);
/* referenced window name, if any */
char *refname pg_node_attr(query_jumble_ignore);
List *partitionClause; /* PARTITION BY list */
/* ORDER BY list */
List *orderClause;
int frameOptions; /* frame_clause options, see WindowDef */
Node *startOffset; /* expression for starting bound, if any */
Node *endOffset; /* expression for ending bound, if any */
.....
}
On Tue, 9 Jun 2026 at 13:56, jian he <jian.universality@gmail.com> wrote:
Query jumbling considers the ORDER BY clause; therefore, the comment
below is inaccurate./
* The information relevant for the query jumbling is the partition clause
* type and its bounds.
*/
I think the comment should be deleted. It's pretty obvious which
fields are used by looking at the pg_node_attr(query_jumble_ignore)
attributes.
David
On Tue, Jun 9, 2026 at 10:15 AM David Rowley <dgrowleyml@gmail.com> wrote:
On Tue, 9 Jun 2026 at 13:56, jian he <jian.universality@gmail.com> wrote:
Query jumbling considers the ORDER BY clause; therefore, the comment
below is inaccurate./
* The information relevant for the query jumbling is the partition clause
* type and its bounds.
*/I think the comment should be deleted. It's pretty obvious which
fields are used by looking at the pg_node_attr(query_jumble_ignore)
attributes.
I agree.
Let's delete it.
David Rowley <dgrowleyml@gmail.com> writes:
On Tue, 9 Jun 2026 at 13:56, jian he <jian.universality@gmail.com> wrote:
Query jumbling considers the ORDER BY clause; therefore, the comment
below is inaccurate./
* The information relevant for the query jumbling is the partition clause
* type and its bounds.
*/
I think the comment should be deleted. It's pretty obvious which
fields are used by looking at the pg_node_attr(query_jumble_ignore)
attributes.
+1. There are also other comments about query jumbling in nodes/*.h that
seem pretty information-free now. They might have been helpful before
we invented query_jumble_ignore and related annotations, but now they
seem just duplicative.
regards, tom lane
On Wed, 10 Jun 2026 at 04:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
I think the comment should be deleted. It's pretty obvious which
fields are used by looking at the pg_node_attr(query_jumble_ignore)
attributes.+1. There are also other comments about query jumbling in nodes/*.h that
seem pretty information-free now. They might have been helpful before
we invented query_jumble_ignore and related annotations, but now they
seem just duplicative.
Here's a patch for that. I did leave a few comments which mention a
reason why a particular field is ignored. That seems like it could be
useful. I think I've got all the ones that just talk about what's
included or ignored.
One particular comment that I couldn't quite understand was:
"All constants are tracked as
* locations in query jumbling, to be marked as parameters."
Maybe that's talking about some prior method of tagging fields to
jumble. Anyway, it doesn't seem very relevant today, so I got rid of
it.
The header comment for WindowFunc also seems to have become misplaced
due to the IGNORE NULLS work. I shifted that back to the correct
location and removed the surplus jumble comments. Since the comment
then became empty, I wrote something about what the struct is for.
David
Attachments:
cleanup_query_jumble_comments.patchtext/plain; charset=US-ASCII; name=cleanup_query_jumble_comments.patchDownload+10-34
David Rowley <dgrowleyml@gmail.com> writes:
On Wed, 10 Jun 2026 at 04:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
+1. There are also other comments about query jumbling in nodes/*.h that
seem pretty information-free now. They might have been helpful before
we invented query_jumble_ignore and related annotations, but now they
seem just duplicative.
Here's a patch for that. I did leave a few comments which mention a
reason why a particular field is ignored. That seems like it could be
useful. I think I've got all the ones that just talk about what's
included or ignored.
All these changes look good, but I have a few more suggestions,
attached as a delta on top of yours. Notably
* - query_jumble_location: Mark the field as a location to track. This is
- * only allowed for integer fields that include "location" in their name.
+ * only used for fields of type ParseLoc, which otherwise are not jumbled.If you look at how gen_node_support.pl implements that annotation,
my revised statement is correct about the field type, and I don't
see anything that actually constrains the field name to be "location".
Maybe some earlier implementation behaved that way?
regards, tom lane