| From: | Peter Eisentraut <peter(at)eisentraut(dot)org> | 
|---|---|
| To: | Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> | 
| Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Michel Pelletier <pelletier(dot)michel(at)gmail(dot)com> | 
| Subject: | Re: Reducing output size of nodeToString | 
| Date: | 2024-03-20 11:49:52 | 
| Message-ID: | 01e17c3a-b37b-4a08-96f6-0b85be6ffadd@eisentraut.org | 
| Views: | Whole Thread | Raw Message | Download mbox | Resend email | 
| Thread: | |
| Lists: | pgsql-hackers | 
On 19.03.24 17:13, Peter Eisentraut wrote:
> On 11.03.24 21:52, Matthias van de Meent wrote:
>>> * v7-0003-gen_node_support.pl-Mark-location-fields-as-type-.patch
>>>
>>> This looks sensible, but maybe making Location a global type is a bit
>>> much?  Maybe something more specific like ParseLocation, or ParseLoc, to
>>> keep it under 12 characters.
>> I've gone with ParseLoc in the attached v8 patchset.
> 
> I have committed this one.
Next, I was looking at 
v8-0003-pg_node_tree-Don-t-store-query-text-locations-in-.patch.  After 
applying that, I was looking how many uses of nodeToString() (with 
locations) were left.  I think your patch forgot to convert a number of 
them, and there also might have been a few new ones that came in with 
other recent patches.  Might be hard to make sure all new developments 
do this right.  Plus, there are various mentions in the documentation 
that should be updated.  After considering all that, there weren't 
really many callers of nodeToString() left.  It's really only debugging 
support in postgres.c and print.c, and a few places were it doesn't 
matter, like the few places where it initializes "cooked expressions", 
which were in turn already stripped of location fields at some earlier time.
So anyway, my idea was that we should turn this around and make 
nodeToString() always drop location information, and instead add 
nodeToStringWithLocations() for the few debugging uses.  And this would 
also be nice because then it matches exactly with the existing 
stringToNodeWithLocations().
Attached patch shows this.
| Attachment | Content-Type | Size | 
|---|---|---|
| 0001-Do-not-output-actual-value-of-location-fields-in-nod.patch | text/plain | 5.6 KB | 
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ashutosh Bapat | 2024-03-20 11:52:47 | Re: DOCS: add helpful partitioning links | 
| Previous Message | jian he | 2024-03-20 11:46:00 | Re: remaining sql/json patches |