Raw parse tree is not dumped to log

Chao Li <li.evan.chao@gmail.com> 于2025年8月1日周五 13:44写道：

Hi,

When "debug_print_parse" is "on", only the Query structure tree is
dumped, the raw parse tree is not dumped to log. In some cases, viewing raw
parse trees are also helpful. It is very hard to view a tree by watching
variables in a debugger, so I added the following code in my local:

When I debug the code, I care more about Query structure,
debug_print_parse is enough for me.
In the debugger, various tools are available, including nodeToString() and
Python scripts.
I usually use the gdbpg tools in [1]https://github.com/tvondra/gdbpg. But it was too old. It can't support
the newest version well.

[1]: https://github.com/tvondra/gdbpg

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 0cecd464902..ff456d1d94e 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -649,6 +649,10 @@ pg_parse_query(const char *query_string)

TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);

+       if (Debug_print_parse)
+               elog_node_display(LOG, "raw parse tree",
raw_parsetree_list,
+                                                 Debug_pretty_print);
+
return raw_parsetree_list;
}

Before submitting this trivial patch, I still want to confirm with the
community if it's intentional to not dump raw parse tree? If we really
don't want to dump raw parse tree again "debug_print_parse", are you ok to
add a new option "debug_print_raw_parse"?

If you want to add this, I prefer to "debug_print_raw_parse". You should
also add documentation to explain this new GUC.

--
Thanks,
Tender Wang

Before submitting this trivial patch, I still want to confirm with the
community if it's intentional to not dump raw parse tree?

For your reference, here's the past discussion:
/messages/by-id/20080730.172949.132921436.t-ishii@sraoss.co.jp

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Hi Tatsuo, thanks for pointing out the past conversation.

So, the requirement got 1 more vote from me. But to not make noise to
people who are not interested in raw parse tree, I guess it's better to add
a new option "debug_print_raw_parse". For people who are interested in raw
parse tree, turning on a flag once is much more convenient than typing a
command in the debugger for every trace.

I will wait to see if Tom still objects to adding that. I will not make the
code change unless I see a hint of "go".

Chao Li (Evan)
------------------------------
HighGo Software Inc.
https://www.highgo.com/

Tatsuo Ishii <ishii@postgresql.org> 于2025年8月1日周五 15:18写道：

I was reviewing some patches today, and during debugging the patches, I
wanted to view raw parse tree, so I had to apply my local patch of dumping
raw parse for every review, which was so inconvenient.

You may argue that raw parse tree might not be useful for every reviews. I
am still ramping up PG development, I'd like to tune SQL statements and see
differences of resulting parse trees.

So, I made this patch. The change is quiet simple. I just searched for
"Debug_print_parse", and added a new option "Debug_print_raw_parse". Only
when the new option is turned on, raw parse tree will be dumped to logs.
This way will not make noise to people who are not interested in raw parse
tree.

I have run the following tests:

1. In an existing database, edit postgres.conf and add
"debug_print_raw_parse = on", then raw parse tree is dumped to logs as
expected.
2. Init a new database, "debug_print_raw_parse = off" appears in
postgres.conf as expected.
3. "make check" passed

This patches touches config.sgml and rules.sgml, I don't know how to test
the doc changes, any suggestion?

One thing I want reviewer's opinion is that, if start "postgres -d 3", it
originally only turn on debug_print_parse, now it will turn on
debug_print_raw_parse as well, which potentially make people who don't want
raw parse tree unhappy. Maybe use "-d 5" or not turning on
debug_print_raw_parse at all by "-d"? WDYT?

Chao Li (Evan)
------------------------------
HighGo Software Inc.
https://www.highgo.com/

Chao Li <li.evan.chao@gmail.com> 于2025年8月1日周五 16:10写道：

Oh, I forget to attached the patch file, here comes it.

Chao Li (Evan)
------------------------------
HighGo Software Inc.
https://www.highgo.com/

Chao Li <li.evan.chao@gmail.com> 于2025年8月4日周一 15:53写道：

I just noticed that my IDE auto formatted guc_tables.c, which generated a
lot of unnecessary diffs. Recreated the patch, and attach the v2 version.

Chao Li (Evan)
------------------------------
HighGo Software Inc.
https://www.highgo.com/

Chao Li <li.evan.chao@gmail.com> 于2025年8月4日周一 15:57写道：

CommitFests patch created: https://commitfest.postgresql.org/patch/5946/

Updated the patch to v3 version.

Chao Li (Evan)
------------------------------
HighGo Software Inc.
https://www.highgo.com/

Chao Li <li.evan.chao@gmail.com> 于2025年8月5日周二 14:25写道：

Updated the patch to v3 version.

I have looked into this patch.

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 20ccb2d6b54..4370e8307f2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml

Around line 7407 of config.sgml:

These parameters enable various debugging output to be emitted.
When set, they print the resulting parse tree, the query rewriter
output, or the execution plan for each executed query.

I think you should add "the resulting raw parse tree" before "the
resulting parse tree" since the paragraph refers to the each item in
the varlistentry above.

--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -649,6 +649,10 @@ pg_parse_query(const char *query_string)

TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);

+	if (Debug_print_raw_parse)
+		elog_node_display(LOG, "raw parse tree", raw_parsetree_list,
+						  Debug_pretty_print);
+

I'm tempted to change "if (Debug_print_raw_parse)" to:

if (unlikely(Debug_print_raw_parse))

But as we have similar if-statements elsewhere
(e.g. Debug_print_parse), maybe we should keep it for a separate
commit.

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Hi Tatsuo san,

Thank you very much for your review.

On 2025/8/16 13:56, Tatsuo Ishii wrote:

I have looked into this patch.

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 20ccb2d6b54..4370e8307f2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml

Around line 7407 of config.sgml:

I think you should add "the resulting raw parse tree" before "the
resulting parse tree" since the paragraph refers to the each item in
the varlistentry above.

Updated.

--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -649,6 +649,10 @@ pg_parse_query(const char *query_string)

TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);

+	if (Debug_print_raw_parse)
+		elog_node_display(LOG, "raw parse tree", raw_parsetree_list,
+						  Debug_pretty_print);
+

I'm tempted to change "if (Debug_print_raw_parse)" to:

if (unlikely(Debug_print_raw_parse))

But as we have similar if-statements elsewhere
(e.g. Debug_print_parse), maybe we should keep it for a separate
commit.

I have added "unlikely" here. For other places, I can use a separate
commit to add "unlikely".

Best regards,

--

Chao Li (Evan)

------------------------------
HighGo Software Inc.
https://www.highgo.com/

--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -649,6 +649,10 @@ pg_parse_query(const char *query_string)
TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);
+	if (Debug_print_raw_parse)
+ elog_node_display(LOG, "raw parse tree", raw_parsetree_list,
+						  Debug_pretty_print);
+

I'm tempted to change "if (Debug_print_raw_parse)" to:

if (unlikely(Debug_print_raw_parse))

But as we have similar if-statements elsewhere
(e.g. Debug_print_parse), maybe we should keep it for a separate
commit.

I have added "unlikely" here. For other places, I can use a separate
commit to add "unlikely".

--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -649,6 +649,10 @@ pg_parse_query(const char *query_string)

TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);

+ if (unlikely(Debug_print_parse))

This should be:

+ if (unlikely(Debug_print_raw_parse))

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

On 2025/8/18 17:13, Tatsuo Ishii wrote:

+ if (unlikely(Debug_print_parse))

This should be:

+ if (unlikely(Debug_print_raw_parse))

My bad! Such a stupid mistake. Fixed. And I did a test by turning on/off
the flag in postgres.conf, and raw parse tree is only dumped when
"debug_print_raw_parse" is "on".

Best regards,

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

On 2025/8/18 17:13, Tatsuo Ishii wrote:

+ if (unlikely(Debug_print_parse))

This should be:

+ if (unlikely(Debug_print_raw_parse))

My bad! Such a stupid mistake. Fixed. And I did a test by turning
on/off the flag in postgres.conf, and raw parse tree is only dumped
when "debug_print_raw_parse" is "on".

Thanks for updating the patch.
v5 patch looks good to me. If there's no objection, I plan to push the
patch in early September.

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

On Aug 20, 2025, at 09:25, Tatsuo Ishii <ishii@postgresql.org> wrote:

v5 patch looks good to me. If there's no objection, I plan to push the
patch in early September.

Hi Tatsuo san,

Thank you much very for your support.

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

On Wed, Aug 20, 2025 at 8:26 AM Tatsuo Ishii <ishii@postgresql.org> wrote:

Thanks for updating the patch.
v5 patch looks good to me. If there's no objection, I plan to push the
patch in early September.

+ if (unlikely(Debug_print_raw_parse))

Branch alignment surely shouldn't matter in a function that is called
once per query?

--
John Naylor
Amazon Web Services

+ if (unlikely(Debug_print_raw_parse))

Branch alignment surely shouldn't matter in a function that is called
once per query?

According to my test, it seems using unlikely() makes a small but
non-negligible performance improvement over the code without
unlikely() for small queries.

Test method: Apply the latest patch (it needs rebasing) to master.
Disable debug_print_raw_parse. Run pgbench with following arguments:

$ pgbench -c 10 -j 10 -f bench.sql -T 60 test
cat bench.sql
SELECT 1;

I ran pgbench 3 times and here's the results:

case 1: with patch (unlikely() is used)
tps = 63279.645198 (without initial connection time)
tps = 62385.016698 (without initial connection time)
tps = 62950.431703 (without initial connection time)
average: 62871.697866

case 2: with patch (unlikely() is not used)
tps = 62462.798551 (without initial connection time)
tps = 63085.646278 (without initial connection time)
tps = 61361.373551 (without initial connection time)
average: 62303.272793

case 1 (62871.697866) / case 2 (62303.272793) = 1.009213

It seems case 1 (unlikely() is used) is about 0.9% faster than case
2 (unlikely() is used).

Comments?
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Hi Chao Li,

v5 patch looks good to me. If there's no objection, I plan to push the
patch in early September.

Hi Tatsuo san,

Thank you much very for your support.

You are welcome.

Can you please rebase v5 patch? It does not apply to current master
anymore.

$ git apply ~/v5-0001-Add-support-for-dumping-raw-parse-tree-with-debug.patch
error: patch failed: src/backend/utils/misc/guc_tables.c:1385
error: src/backend/utils/misc/guc_tables.c: patch does not apply

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Hi Tatsuo san,

Rebased v6 is attached.

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

Tatsuo Ishii <ishii@postgresql.org> 于2025年9月1日周一 10:13写道：

On Mon, Sep 1, 2025 at 8:13 AM Tatsuo Ishii <ishii@postgresql.org> wrote:

Branch alignment surely shouldn't matter in a function that is called
once per query?

According to my test, it seems using unlikely() makes a small but
non-negligible performance improvement over the code without
unlikely() for small queries.

Test method: Apply the latest patch (it needs rebasing) to master.
Disable debug_print_raw_parse. Run pgbench with following arguments:

$ pgbench -c 10 -j 10 -f bench.sql -T 60 test
cat bench.sql
SELECT 1;

I ran pgbench 3 times and here's the results:

case 1: with patch (unlikely() is used)
tps = 63279.645198 (without initial connection time)
tps = 62385.016698 (without initial connection time)
tps = 62950.431703 (without initial connection time)
average: 62871.697866

case 2: with patch (unlikely() is not used)
tps = 62462.798551 (without initial connection time)
tps = 63085.646278 (without initial connection time)
tps = 61361.373551 (without initial connection time)
average: 62303.272793

case 1 (62871.697866) / case 2 (62303.272793) = 1.009213

It seems case 1 (unlikely() is used) is about 0.9% faster than case
2

The individual runs have quite a bit of variation.

It's good to cross check results against known facts. If my napkin
math is right, a ~1% speedup for ~60ktps/s amounts to saving ~160ns
per query. It's not plausible that forcing the compiler's hand for
this branch would save several hundred clock cycles.

To be fair I tried to reproduce and found only noise-level differences:
master: 61553
case 1: 61423
case 2: 61647

--
John Naylor
Amazon Web Services