Horton

Thread

Commits

  1. Fix various query jumble comments

  1. typedef struct WindowClause misleading comments

    jian he <jian.universality@gmail.com> — 2026-06-09T01:55:47Z

    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 */
   .....
}

  2. Re: typedef struct WindowClause misleading comments

    David Rowley <dgrowleyml@gmail.com> — 2026-06-09T02:15:01Z

    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

  3. Re: typedef struct WindowClause misleading comments

    jian he <jian.universality@gmail.com> — 2026-06-09T06:11:57Z

    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.

  4. Re: typedef struct WindowClause misleading comments

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-06-09T16:54:49Z

    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

  5. Re: typedef struct WindowClause misleading comments

    David Rowley <dgrowleyml@gmail.com> — 2026-06-12T02:50:27Z

    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

  6. Re: typedef struct WindowClause misleading comments

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-06-12T16:15:32Z

    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

  7. Re: typedef struct WindowClause misleading comments

    David Rowley <dgrowleyml@gmail.com> — 2026-06-16T01:44:23Z

    On Sat, 13 Jun 2026 at 04:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> All these changes look good, but I have a few more suggestions,
> attached as a delta on top of yours.  Notably

Thank you. I've included your changes and pushed.

David