Fixes a trivial bug in dumped parse/query/plan trees

Started by Chao Li5 months ago8 messages
#1Chao Li
li.evan.chao@gmail.com
1 attachment(s)

Hi Hackers,

This patch fixes a trivial bug where an extra whitespace was added
when dumping an array, for example:

```
:sort.numCols 2
:sort.sortColIdx ( 1 4)
:sort.sortOperators ( 97 1754)
:sort.collations ( 0 0)
:sort.nullsFirst ( false false)
```

The unnecessary whitespace is now removed.

Regard regards,

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachments:

v1-0001-Fixes-a-trivial-bug-in-dumped-parse-query-plan-tr.patchapplication/octet-stream; name=v1-0001-Fixes-a-trivial-bug-in-dumped-parse-query-plan-tr.patch; x-unix-mode=0644Download
From 6fc48134292d1ce699143f7d3d2e81c5aed3b7bd Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <lic@highgo.com>
Date: Mon, 25 Aug 2025 08:42:03 +0800
Subject: [PATCH v1] Fixes a trivial bug in dumped parse/query/plan trees

This patch fixes a trivial bug where an extra whitespace was added
when dumping an array, for example:

```
  :sort.numCols 2
  :sort.sortColIdx ( 1 4)
  :sort.sortOperators ( 97 1754)
  :sort.collations ( 0 0)
  :sort.nullsFirst ( false false)
```

The unnecessary whitespace is now removed.

Author: Li Chao <lic@highgo.com>
---
 src/backend/nodes/outfuncs.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index eaf391fc2ab..be390d42723 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -238,18 +238,22 @@ fnname(StringInfo str, const datatype *arr, int len) \
 	{ \
 		appendStringInfoChar(str, '('); \
 		for (int i = 0; i < len; i++) \
+		{ \
 			appendStringInfo(str, fmtstr, convfunc(arr[i])); \
+			if (i < len - 1) \
+				appendStringInfoChar(str, ' '); \
+		} \
 		appendStringInfoChar(str, ')'); \
 	} \
 	else \
 		appendStringInfoString(str, "<>"); \
 }
 
-WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, " %d",)
-WRITE_SCALAR_ARRAY(writeOidCols, Oid, " %u",)
-WRITE_SCALAR_ARRAY(writeIndexCols, Index, " %u",)
-WRITE_SCALAR_ARRAY(writeIntCols, int, " %d",)
-WRITE_SCALAR_ARRAY(writeBoolCols, bool, " %s", booltostr)
+WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, "%d",)
+WRITE_SCALAR_ARRAY(writeOidCols, Oid, "%u",)
+WRITE_SCALAR_ARRAY(writeIndexCols, Index, "%u",)
+WRITE_SCALAR_ARRAY(writeIntCols, int, "%d",)
+WRITE_SCALAR_ARRAY(writeBoolCols, bool, "%s", booltostr)
 
 /*
  * Print an array (not a List) of Node pointers.
-- 
2.39.5 (Apple Git-154)

#2Stepan Neretin
slpmcf@gmail.com
In reply to: Chao Li (#1)
Re: Fixes a trivial bug in dumped parse/query/plan trees

On Mon, Aug 25, 2025 at 7:55 AM Chao Li <li.evan.chao@gmail.com> wrote:

Hi Hackers,

This patch fixes a trivial bug where an extra whitespace was added
when dumping an array, for example:

```
:sort.numCols 2
:sort.sortColIdx ( 1 4)
:sort.sortOperators ( 97 1754)
:sort.collations ( 0 0)
:sort.nullsFirst ( false false)
```

The unnecessary whitespace is now removed.

Regard regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi Chao,

I was able to reproduce the problem using a small test:

```sql
SET debug_print_parse = on;

CREATE TABLE test_sort_example (
id SERIAL PRIMARY KEY,
col1 INT,
col2 TEXT,
col3 TEXT
);

INSERT INTO test_sort_example (col1, col2, col3) VALUES
(1, 'apple', 'zebra'),
(2, 'banana', 'yak'),
(3, 'cherry', 'xylophone'),
(4, 'date', 'wolf');

SELECT * FROM test_sort_example
ORDER BY col2 ASC, col3 ASC;
```

The extra whitespace in the array dump (e.g.,

```
:sort.numCols 2
:sort.sortColIdx ( 1 4)
:sort.sortOperators ( 97 1754)
:sort.collations ( 0 0)
:sort.nullsFirst ( false false)
```

) was indeed present before the patch.

After applying your patch, the issue is fixed, and the array is now dumped
correctly without extra spaces. The patch looks good to me.

Regarding testing: for this case, a Perl test might actually be more
suitable than SQL tests, because OIDs can change between runs and Perl
tests allow for better control and normalization of such values.

Best regards,
Stepan Neretin

#3Chao Li
li.evan.chao@gmail.com
In reply to: Stepan Neretin (#2)
Re: Fixes a trivial bug in dumped parse/query/plan trees

Hi Stepan,

Thanks for your testing.

On Aug 25, 2025, at 11:20, Stepan Neretin <slpmcf@gmail.com> wrote:

Regarding testing: for this case, a Perl test might actually be more suitable than SQL tests, because OIDs can change between runs and Perl tests allow for better control and normalization of such values.

Are you suggesting add a test case to this patch? Can you please be more specific?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

#4Daniel Gustafsson
daniel@yesql.se
In reply to: Chao Li (#1)
Re: Fixes a trivial bug in dumped parse/query/plan trees

On 25 Aug 2025, at 02:54, Chao Li <li.evan.chao@gmail.com> wrote:

Hi Hackers,
This patch fixes a trivial bug where an extra whitespace was added
when dumping an array, for example:

```
:sort.numCols 2
:sort.sortColIdx ( 1 4)
:sort.sortOperators ( 97 1754)
:sort.collations ( 0 0)
:sort.nullsFirst ( false false)
```

The unnecessary whitespace is now removed.

What about external parsers written for this format which might now break?
(Judging by the commitlog I believe this format is intentional and not a bug.)

--
Daniel Gustafsson

#5Chao Li
li.evan.chao@gmail.com
In reply to: Daniel Gustafsson (#4)
Re: Fixes a trivial bug in dumped parse/query/plan trees

On Aug 25, 2025, at 16:02, Daniel Gustafsson <daniel@yesql.se> wrote:

What about external parsers written for this format which might now break?
(Judging by the commitlog I believe this format is intentional and not a bug.)

The commit message says:

Let's adopt a format similar to that used for Lists, that is "<>"
for a NULL pointer or "(item item item)" otherwise. Also retool
the code to not have so many copies of the identical logic.

It mentioned “(item item item)”, not a whitespace in front of the first item.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

#6Chao Li
li.evan.chao@gmail.com
In reply to: Chao Li (#5)
1 attachment(s)
Re: Fixes a trivial bug in dumped parse/query/plan trees

On Aug 25, 2025, at 16:57, Chao Li <li.evan.chao@gmail.com> wrote:

Resending the patch file.

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachments:

v1-0001-Fixes-a-trivial-bug-in-dumped-parse-query-plan-tr.patchapplication/octet-stream; name=v1-0001-Fixes-a-trivial-bug-in-dumped-parse-query-plan-tr.patch; x-unix-mode=0644Download
From 6fc48134292d1ce699143f7d3d2e81c5aed3b7bd Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <lic@highgo.com>
Date: Mon, 25 Aug 2025 08:42:03 +0800
Subject: [PATCH v1] Fixes a trivial bug in dumped parse/query/plan trees

This patch fixes a trivial bug where an extra whitespace was added
when dumping an array, for example:

```
  :sort.numCols 2
  :sort.sortColIdx ( 1 4)
  :sort.sortOperators ( 97 1754)
  :sort.collations ( 0 0)
  :sort.nullsFirst ( false false)
```

The unnecessary whitespace is now removed.

Author: Li Chao <lic@highgo.com>
---
 src/backend/nodes/outfuncs.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index eaf391fc2ab..be390d42723 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -238,18 +238,22 @@ fnname(StringInfo str, const datatype *arr, int len) \
 	{ \
 		appendStringInfoChar(str, '('); \
 		for (int i = 0; i < len; i++) \
+		{ \
 			appendStringInfo(str, fmtstr, convfunc(arr[i])); \
+			if (i < len - 1) \
+				appendStringInfoChar(str, ' '); \
+		} \
 		appendStringInfoChar(str, ')'); \
 	} \
 	else \
 		appendStringInfoString(str, "<>"); \
 }
 
-WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, " %d",)
-WRITE_SCALAR_ARRAY(writeOidCols, Oid, " %u",)
-WRITE_SCALAR_ARRAY(writeIndexCols, Index, " %u",)
-WRITE_SCALAR_ARRAY(writeIntCols, int, " %d",)
-WRITE_SCALAR_ARRAY(writeBoolCols, bool, " %s", booltostr)
+WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, "%d",)
+WRITE_SCALAR_ARRAY(writeOidCols, Oid, "%u",)
+WRITE_SCALAR_ARRAY(writeIndexCols, Index, "%u",)
+WRITE_SCALAR_ARRAY(writeIntCols, int, "%d",)
+WRITE_SCALAR_ARRAY(writeBoolCols, bool, "%s", booltostr)
 
 /*
  * Print an array (not a List) of Node pointers.
-- 
2.39.5 (Apple Git-154)

#7Chao Li
li.evan.chao@gmail.com
In reply to: Chao Li (#6)
1 attachment(s)
Re: Fixes a trivial bug in dumped parse/query/plan trees

Retry again to attach the path file.

Chao Li (Evan)
---------------------
Highgo Software Co., Ltd.
https://www.highgo.com/

Chao Li <li.evan.chao@gmail.com> 于2025年8月27日周三 09:35写道:

Show quoted text

On Aug 25, 2025, at 16:57, Chao Li <li.evan.chao@gmail.com> wrote:

Resending the patch file.

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachments:

v1-0001-Fixes-a-trivial-bug-in-dumped-parse-query-plan-tr.patchapplication/octet-stream; name=v1-0001-Fixes-a-trivial-bug-in-dumped-parse-query-plan-tr.patchDownload
From 6fc48134292d1ce699143f7d3d2e81c5aed3b7bd Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <lic@highgo.com>
Date: Mon, 25 Aug 2025 08:42:03 +0800
Subject: [PATCH v1] Fixes a trivial bug in dumped parse/query/plan trees

This patch fixes a trivial bug where an extra whitespace was added
when dumping an array, for example:

```
  :sort.numCols 2
  :sort.sortColIdx ( 1 4)
  :sort.sortOperators ( 97 1754)
  :sort.collations ( 0 0)
  :sort.nullsFirst ( false false)
```

The unnecessary whitespace is now removed.

Author: Li Chao <lic@highgo.com>
---
 src/backend/nodes/outfuncs.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index eaf391fc2ab..be390d42723 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -238,18 +238,22 @@ fnname(StringInfo str, const datatype *arr, int len) \
 	{ \
 		appendStringInfoChar(str, '('); \
 		for (int i = 0; i < len; i++) \
+		{ \
 			appendStringInfo(str, fmtstr, convfunc(arr[i])); \
+			if (i < len - 1) \
+				appendStringInfoChar(str, ' '); \
+		} \
 		appendStringInfoChar(str, ')'); \
 	} \
 	else \
 		appendStringInfoString(str, "<>"); \
 }
 
-WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, " %d",)
-WRITE_SCALAR_ARRAY(writeOidCols, Oid, " %u",)
-WRITE_SCALAR_ARRAY(writeIndexCols, Index, " %u",)
-WRITE_SCALAR_ARRAY(writeIntCols, int, " %d",)
-WRITE_SCALAR_ARRAY(writeBoolCols, bool, " %s", booltostr)
+WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, "%d",)
+WRITE_SCALAR_ARRAY(writeOidCols, Oid, "%u",)
+WRITE_SCALAR_ARRAY(writeIndexCols, Index, "%u",)
+WRITE_SCALAR_ARRAY(writeIntCols, int, "%d",)
+WRITE_SCALAR_ARRAY(writeBoolCols, bool, "%s", booltostr)
 
 /*
  * Print an array (not a List) of Node pointers.
-- 
2.39.5 (Apple Git-154)

#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Daniel Gustafsson (#4)
Re: Fixes a trivial bug in dumped parse/query/plan trees

Daniel Gustafsson <daniel@yesql.se> writes:

On 25 Aug 2025, at 02:54, Chao Li <li.evan.chao@gmail.com> wrote:
This patch fixes a trivial bug where an extra whitespace was added
when dumping an array, for example:
...
The unnecessary whitespace is now removed.

What about external parsers written for this format which might now break?
(Judging by the commitlog I believe this format is intentional and not a bug.)

Yeah, I'm inclined to reject this patch. This is by no stretch of the
imagination a "bug"; a more accurate characterization is that it's a
minor optimization that the original author did not think was worth
troubling with. While our existing nodeRead code wouldn't have a
problem with removing the extra whitespace, I share your fear that
we could introduce bugs into some third-party code somewhere.
So the cost/benefit ratio of making the change doesn't look very good
to me.

I do have interest in trying to aggressively reduce the stored size
of view/rule parsetrees, and would be willing to break such
hypothetical third-party code in pursuit of a sufficiently large
improvement (say, 2X or better). But a change like this one,
in isolation, isn't going to move that needle.

regards, tom lane