Feedback-ID: i131946ab:Fastmail
Message-ID: <e4801c28-cb5b-3b81-bda1-03af8df1fef9@enterprisedb.com>
Date: Tue, 17 Jan 2023 08:43:44 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: Generating code for query jumbling through gen_node_support.pl
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
 "Drouvot, Bertrand" <bdrouvot@amazon.com>
References: <Y5BHOUhX3zTH/ig6@paquier.xyz>
 <34491a4e-2664-6ea7-662c-b13d18d2a92d@enterprisedb.com>
 <Y8EOUuViEL8Ewvf/@paquier.xyz>
 <d74d2681-5c07-2f6e-d177-995905015311@enterprisedb.com>
 <Y8YakMwx2a6Y63oR@paquier.xyz>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <Y8YakMwx2a6Y63oR@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/e4801c28-cb5b-3b81-bda1-03af8df1fef9%40enterprisedb.com>
Precedence: bulk

On 17.01.23 04:48, Michael Paquier wrote:
>>> Anyway, when it comes to the location, another thing that can be
>>> considered here would be to require a node-level flag for the nodes on
>>> which we want to track the location.  This overlaps a bit with the
>>> fields satisfying "($t eq 'int' && $f =~ 'location$')", but it removes
>>> most of the code changes like this one as at the end we only care
>>> about the location for Const nodes:
>>> -   int         location;       /* token location, or -1 if unknown */
>>> +   int         location pg_node_attr(jumble_ignore);   /* token location, or -1
>>> +                                                        * if unknown */
>>>
>>> I have taken this approach in v2 of the patch, shaving ~100 lines of
>>> more code as there is no need to mark all these location fields with
>>> "jumble_ignore" anymore, except for Const, of course.
>>
>> I don't understand why you chose that when we already have an established
>> way.  This would just make the jumble annotations inconsistent with the
>> other annotations.
> 
> Because we don't want to track the location of all the nodes!  If we
> do that, pg_stat_statements would begin to parameterize a lot more
> things, from column aliases to full contents of IN or WITH clauses.
> The root point is that we only want to track the jumble location for
> Const nodes now.  In order to do that, there are two approaches:
> - Mark all the locations with jumble_ignore: more invasive, but
> it requires only one per-field attribute with "jumble_ignore".  This
> is what v1 did.

Ok, I understand now, and I agree with this approach over the opposite. 
I was confused because the snippet you showed above used 
"jumble_ignore", but your patch is correct as it uses "jumble_location".

That said, the term "jumble" is really weird, because in the sense that 
we are using it here it means, approximately, "to mix together", "to 
unify".  So what we are doing with the Const nodes is really to *not* 
jumble the location, but for all other node types we are jumbling the 
location.  At least that is my understanding.