Обсуждение: Raw parse tree is not dumped to log

> 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:
https://www.postgresql.org/message-id/flat/20080730.172949.132921436.t-ishii%40sraoss.co.jp

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

> 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.

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)

>> --- 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

 
+	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 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

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

> The individual runs have quite a bit of variation.

It's a known behavior of pgbench.

> 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

That's 0.3% difference, definitely noise-level. Ok, I withdraw my
claim and commit the patch without unlikely().

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

Hi,

> Hi Tatsuo san,
> 
> I just rebased the commit for the GUC change.
> 
> On Sep 3, 2025, at 09:57, Tatsuo Ishii <ishii@postgresql.org> wrote:
> 
> That's 0.3% difference, definitely noise-level. Ok, I withdraw my
> claim and commit the patch without unlikely().
> 
> I also removed unlikely() in v7.

v7 patch looks good to me. BTW, how do you want to be credited in the
commit message?

Author: Chao Li <lic@highgo.com>
Author: Chao Li <li.evan.chao@gmail.com>

It seems your email is the latter, however you wrote the former in
your patch.

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

>> v7 patch looks good to me. BTW, how do you want to be credited in the
>> commit message?
>> 
>> Author: Chao Li <lic@highgo.com <mailto:lic@highgo.com>>
>> Author: Chao Li <li.evan.chao@gmail.com <mailto:li.evan.chao@gmail.com>>
>> 
>> It seems your email is the latter, however you wrote the former in
>> your patch.
>> 
> 
> Please use the former one that is my company email address. Because it doesn’t work very well with the Mail Archive,
soI use my personal Gmail for daily community communication.
 

Okay. I have just pushed v7 patch. Thank you for your work!
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp