Extending outfuncs support to utility statements

Hi,

On 2022-07-09 18:20:26 -0400, Tom Lane wrote:

We've long avoided building I/O support for utility-statement node
types, mainly because it didn't seem worth the trouble to write and
maintain such code by hand. Now that the automatic node-support-code
generation patch is in, that argument is gone, and it's just a matter
of whether the benefits are worth the backend code bloat. I can
see two benefits worth considering:

* Seems like having such support would be pretty useful for
debugging.

Agreed.

So I looked into how much code are we talking about. On my
RHEL8 x86_64 machine, the code sizes for outfuncs/readfuncs
as of HEAD are

$ size outfuncs.o readfuncs.o
text data bss dec hex filename
117173 0 0 117173 1c9b5 outfuncs.o
64540 0 0 64540 fc1c readfuncs.o

If we just open the floodgates and enable both outfuncs and
readfuncs support for all *Stmt nodes (plus some node types
that thereby become dumpable, like AlterTableCmd), then
this becomes

$ size outfuncs.o readfuncs.o
text data bss dec hex filename
139503 0 0 139503 220ef outfuncs.o
95562 0 0 95562 1754a readfuncs.o

For my taste, the circa 20K growth in outfuncs.o is an okay
price for being able to inspect utility statements more easily.
However, I'm less thrilled with the 30K growth in readfuncs.o,
because I can't see that we'd get any direct benefit from that.
So I think a realistic proposal is to enable outfuncs support
but keep readfuncs disabled.

Another approach could be to mark those paths as "cold", so they are placed
further away, reducing / removing potential overhead due to higher iTLB misses
etc. 30K of disk space isn't worth worrying about.

Don't really have an opinion on this.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-07-09 18:20:26 -0400, Tom Lane wrote:

For my taste, the circa 20K growth in outfuncs.o is an okay
price for being able to inspect utility statements more easily.
However, I'm less thrilled with the 30K growth in readfuncs.o,
because I can't see that we'd get any direct benefit from that.
So I think a realistic proposal is to enable outfuncs support
but keep readfuncs disabled.

Another approach could be to mark those paths as "cold", so they are placed
further away, reducing / removing potential overhead due to higher iTLB misses
etc. 30K of disk space isn't worth worrying about.

They're not so much "cold" as "dead", so I don't see the point
of having them at all. If we ever start allowing utility commands
(besides NOTIFY) in stored rules, we'd need readfuncs support then
... but at least in the short run I don't see that happening.

regards, tom lane

Hi,

On 2022-07-10 19:12:52 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2022-07-09 18:20:26 -0400, Tom Lane wrote:

For my taste, the circa 20K growth in outfuncs.o is an okay
price for being able to inspect utility statements more easily.
However, I'm less thrilled with the 30K growth in readfuncs.o,
because I can't see that we'd get any direct benefit from that.
So I think a realistic proposal is to enable outfuncs support
but keep readfuncs disabled.

Another approach could be to mark those paths as "cold", so they are placed
further away, reducing / removing potential overhead due to higher iTLB misses
etc. 30K of disk space isn't worth worrying about.

They're not so much "cold" as "dead", so I don't see the point
of having them at all. If we ever start allowing utility commands
(besides NOTIFY) in stored rules, we'd need readfuncs support then
... but at least in the short run I don't see that happening.

It would allow us to test utility outfuncs as part of the
WRITE_READ_PARSE_PLAN_TREES check. Not that that's worth very much.

I guess it could be a minor help in making a few more utility commands benefit
from paralellism?

Anyway, as mentioned earlier, I'm perfectly fine not supporting readfuns for
utility statements for now.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-07-10 19:12:52 -0400, Tom Lane wrote:

They're not so much "cold" as "dead", so I don't see the point
of having them at all. If we ever start allowing utility commands
(besides NOTIFY) in stored rules, we'd need readfuncs support then
... but at least in the short run I don't see that happening.

It would allow us to test utility outfuncs as part of the
WRITE_READ_PARSE_PLAN_TREES check. Not that that's worth very much.

Especially now that those are all auto-generated anyway.

I guess it could be a minor help in making a few more utility commands benefit
from paralellism?

Again, once we have an actual use-case, enabling that code will be
fine by me. But we don't yet.

regards, tom lane

Hi,

Now we are ready to have debug_print_raw_parse (or something like
that)? Pgpool-II has been importing and using PostgreSQL's raw
parser for years. I think it would be great for PostgreSQL and
Pgpool-II developers to have such a feature.

Best reagards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On 10.07.22 00:20, Tom Lane wrote:

We've long avoided building I/O support for utility-statement node
types, mainly because it didn't seem worth the trouble to write and
maintain such code by hand. Now that the automatic node-support-code
generation patch is in, that argument is gone, and it's just a matter
of whether the benefits are worth the backend code bloat. I can
see two benefits worth considering:

This is also needed to be able to store utility statements in (unquoted)
SQL function bodies. I have some in-progress code for that that I need
to dust off. IIRC, there are still some nontrivial issues to work
through on the reading side. I don't have a problem with enabling the
outfuncs side in the meantime.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

On 10.07.22 00:20, Tom Lane wrote:

We've long avoided building I/O support for utility-statement node
types, mainly because it didn't seem worth the trouble to write and
maintain such code by hand.

k

This is also needed to be able to store utility statements in (unquoted)
SQL function bodies. I have some in-progress code for that that I need
to dust off. IIRC, there are still some nontrivial issues to work
through on the reading side. I don't have a problem with enabling the
outfuncs side in the meantime.

Oh! I'd not thought of that, but yes that is a plausible near-term
requirement for readfuncs support for utility statements. So my
concern about suppressing those is largely a waste of effort.

There might be enough node types that are raw-parse-tree-only,
but not involved in utility statements, to make it worth
continuing to suppress readfuncs support for them. But I kinda
doubt it. I'll try to get some numbers later today.

regards, tom lane

I wrote:

There might be enough node types that are raw-parse-tree-only,
but not involved in utility statements, to make it worth
continuing to suppress readfuncs support for them. But I kinda
doubt it. I'll try to get some numbers later today.

Granting that we want write/read support for utility statements,
it seems that what we can save by suppressing raw-parse-tree-only
nodes is only about 10kB. That's clearly not worth troubling over
in the grand scheme of things, so I suggest that we just open the
floodgates as attached.

regards, tom lane

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

This is also needed to be able to store utility statements in (unquoted)
SQL function bodies. I have some in-progress code for that that I need
to dust off. IIRC, there are still some nontrivial issues to work
through on the reading side. I don't have a problem with enabling the
outfuncs side in the meantime.

BTW, I experimented with trying to enable WRITE_READ_PARSE_PLAN_TREES
for utility statements, and found that the immediate problem is that
Constraint and a couple of other node types lack read functions
(they're the ones marked "custom_read_write, no_read" in parsenodes.h).
They have out functions, so writing the inverses seems like it's just
something nobody ever got around to. Perhaps there are deeper problems
lurking behind that one, though.

regards, tom lane

On 13.07.22 00:38, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

This is also needed to be able to store utility statements in (unquoted)
SQL function bodies. I have some in-progress code for that that I need
to dust off. IIRC, there are still some nontrivial issues to work
through on the reading side. I don't have a problem with enabling the
outfuncs side in the meantime.

BTW, I experimented with trying to enable WRITE_READ_PARSE_PLAN_TREES
for utility statements, and found that the immediate problem is that
Constraint and a couple of other node types lack read functions
(they're the ones marked "custom_read_write, no_read" in parsenodes.h).
They have out functions, so writing the inverses seems like it's just
something nobody ever got around to. Perhaps there are deeper problems
lurking behind that one, though.

Here are patches for that.

v1-0001-Fix-reading-of-most-negative-integer-value-nodes.patch
v1-0002-Fix-reading-of-BitString-nodes.patch

These are some of those lurking problems.

v1-0003-Add-read-support-for-some-missing-raw-parse-nodes.patch

This adds the read support for the missing nodes.

The above patches are candidates for committing.

At this point we have one structural problem left: char * node fields
output with WRITE_STRING_FIELD() (ultimately outToken()) don't
distinguish between empty strings and NULL values. A write/read
roundtrip ends up as NULL for an empty string. This shows up in the
regression tests for commands such as

CREATE TABLESPACE regress_tblspace LOCATION '';
CREATE SUBSCRIPTION regress_addr_sub CONNECTION '' ...

This will need some expansion of the output format to handle this.

v1-0004-XXX-Turn-on-WRITE_READ_PARSE_PLAN_TREES-for-testi.patch
v1-0005-Implement-WRITE_READ_PARSE_PLAN_TREES-for-raw-par.patch
v1-0006-Enable-WRITE_READ_PARSE_PLAN_TREES-of-rewritten-u.patch

This is for testing the above. Note that in 0005 we need some special
handling for float values to preserve the full precision across
write/read. I suppose this could be unified with the code the preserves
the location fields when doing write/read checking.

v1-0007-Enable-utility-statements-in-unquoted-SQL-functio.patch

This demonstrates what the ultimate goal is. A few more tests should be
added eventually.

Hi,

On 2022-08-24 17:25:31 +0200, Peter Eisentraut wrote:

Here are patches for that.

These patches have been failing since they were posted, afaict:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3848

I assume that's known? Most of the failures seem to be things like
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/tablespace.out /tmp/cirrus-ci-build/build/testrun/main/regress/results/tablespace.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/tablespace.out	2022-09-22 12:30:07.340655000 +0000
+++ /tmp/cirrus-ci-build/build/testrun/main/regress/results/tablespace.out	2022-09-22 12:35:15.075825000 +0000
@@ -3,6 +3,8 @@
 ERROR:  tablespace location must be an absolute path
 -- empty tablespace locations are not usually allowed
 CREATE TABLESPACE regress_tblspace LOCATION ''; -- fail
+WARNING:  outfuncs/readfuncs failed to produce an equal raw parse tree
+WARNING:  outfuncs/readfuncs failed to produce an equal rewritten parse tree
 ERROR:  tablespace location must be an absolute path
 -- as a special developer-only option to allow us to use tablespaces
 -- with streaming replication on the same server, an empty location

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-08-24 17:25:31 +0200, Peter Eisentraut wrote:

Here are patches for that.

These patches have been failing since they were posted, afaict:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3848

I assume that's known?

I think this is the issue Peter mentioned about needing to distinguish
between empty strings and NULL strings. We're going to need to rethink
the behavior of pg_strtok() a bit to fix that.

regards, tom lane

I wrote:

I think this is the issue Peter mentioned about needing to distinguish
between empty strings and NULL strings. We're going to need to rethink
the behavior of pg_strtok() a bit to fix that.

After staring at the code a bit, I think we don't need to touch
pg_strtok() per se. I propose that this can be resolved with changes
at the next higher level. Let's make outToken print NULL as <> as
it always has, but print an empty string as "" (two double quotes).
If the raw input string is two double quotes, print it as \"" to
disambiguate. This'd require a catversion bump when committed,
but I don't think there are any showstopper problems otherwise.

I'll work on fleshing that idea out.

regards, tom lane

On 2022-09-22 12:48:47 -0400, Tom Lane wrote:

I wrote:

I think this is the issue Peter mentioned about needing to distinguish
between empty strings and NULL strings. We're going to need to rethink
the behavior of pg_strtok() a bit to fix that.

After staring at the code a bit, I think we don't need to touch
pg_strtok() per se. I propose that this can be resolved with changes
at the next higher level. Let's make outToken print NULL as <> as
it always has, but print an empty string as "" (two double quotes).
If the raw input string is two double quotes, print it as \"" to
disambiguate. This'd require a catversion bump when committed,
but I don't think there are any showstopper problems otherwise.

Makes sense to me.

Andres Freund <andres@anarazel.de> writes:

On 2022-09-22 12:48:47 -0400, Tom Lane wrote:

After staring at the code a bit, I think we don't need to touch
pg_strtok() per se. I propose that this can be resolved with changes
at the next higher level. Let's make outToken print NULL as <> as
it always has, but print an empty string as "" (two double quotes).
If the raw input string is two double quotes, print it as \"" to
disambiguate. This'd require a catversion bump when committed,
but I don't think there are any showstopper problems otherwise.

Makes sense to me.

Here is a version of all-but-the-last patch in Peter's series.
I left off the last one because it fails check-world: we now
get through the core regression tests okay, but then the pg_dump
tests fail on the new SQL function. To fix that, we would have
to extend ruleutils.c's get_utility_query_def() to be able to
fully reconstruct any legal utility query ... which seems like
a pretty dauntingly large amount of tedious manual effort to
start with, and then also a nontrivial additional requirement
on any future patch that adds new utility syntax. Are we sure
it's worth going there?

But I think it's probably worth committing what we have here
just on testability grounds.

Some notes:

0001, 0002 not changed.

I tweaked 0003 a bit, mainly because I think it's probably not very
safe to apply strncmp to a string we don't know the length of.
It might be difficult to fall off the end of memory that way, but
I wouldn't bet it's impossible. Also, adding the length checks gets
rid of the need for a grotty order dependency in _readA_Expr().

0004 fixes the empty-string problem as per above.

I did not like what you'd done about imprecise floats one bit.
I think we ought to do it as in 0005 instead: drop all the
hard-wired precision assumptions and just print per Ryu.

0006, 0007, 0008 are basically the same as your previous 0004,
0005, 0006, except for getting rid of the float hacking in 0005.

If you're good with this approach to the float issue, I think
this set is committable (minus 0006 of course, and don't forget
the catversion bump).

regards, tom lane

I wrote:

I left off the last one because it fails check-world: we now
get through the core regression tests okay, but then the pg_dump
tests fail on the new SQL function. To fix that, we would have
to extend ruleutils.c's get_utility_query_def() to be able to
fully reconstruct any legal utility query ... which seems like
a pretty dauntingly large amount of tedious manual effort to
start with, and then also a nontrivial additional requirement
on any future patch that adds new utility syntax. Are we sure
it's worth going there?

Thinking about that some more, I wondered if we'd even wish to
build such code, compared to just saving the original source text
for utility statements and printing that. Obviously, this loses
all the benefits of new-style SQL functions compared to old-style
... except that those benefits would be illusory anyway, since by
definition we have not done parse analysis on a utility statement.
So we *cannot* offer any useful guarantees about being search_path
change proof, following renames of referenced objects, preventing
drops of referenced objects, etc etc.

This makes me wonder if this is a feature we even want. If we
put it in, we'd have to add a bunch of disclaimers about how
utility statements behave entirely differently from DML statements.

Perhaps an interesting alternative is to allow a command along
the lines of

EXECUTE string-expression

(of course that name is already taken) where we'd parse-analyze
the string-expression at function creation, but then the computed
string is executed as a SQL command in the runtime environment.
This would make it fairly clear which things you have guarantees
of and which you don't. It'd also offer a feature that the PLs
have but SQL functions traditionally haven't, ie execution of
dynamically-computed SQL.

Anyway, this is a bit far afield from the stated topic of this
thread. I think we should commit something approximately like
what I posted and then start a new thread specifically about
what we'd like to do about utility commands in new-style SQL
functions.

regards, tom lane

On 22.09.22 23:21, Tom Lane wrote:

Anyway, this is a bit far afield from the stated topic of this
thread. I think we should commit something approximately like
what I posted and then start a new thread specifically about
what we'd like to do about utility commands in new-style SQL
functions.

Right, I have committed everything and will close the CF entry. I don't
have a specific idea about how to move forward right now.

Hello,

26.09.2022 17:46, Peter Eisentraut wrote:

On 22.09.22 23:21, Tom Lane wrote:

Anyway, this is a bit far afield from the stated topic of this
thread.  I think we should commit something approximately like
what I posted and then start a new thread specifically about
what we'd like to do about utility commands in new-style SQL
functions.

Right, I have committed everything and will close the CF entry.  I don't have a specific idea about how to move
forward right now.

Please look at the function _readA_Const() (introduced in a6bc33019), which fails on current master under valgrind:
CPPFLAGS="-DUSE_VALGRIND -DWRITE_READ_PARSE_PLAN_TREES -Og " ./configure -q --enable-debug && make -s -j8 && make check

============== creating temporary instance            ==============
============== initializing database system           ==============

pg_regress: initdb failed
Examine .../src/test/regress/log/initdb.log for the reason.

initdb.log contains:
performing post-bootstrap initialization ... ==00:00:00:02.155 3419654== Invalid read of size 16
==00:00:00:02.155 3419654==    at 0x448691: memcpy (string_fortified.h:29)
==00:00:00:02.155 3419654==    by 0x448691: _readA_Const (readfuncs.c:315)
==00:00:00:02.155 3419654==    by 0x44CCD2: parseNodeString (readfuncs.switch.c:129)
==00:00:00:02.155 3419654==    by 0x4348D6: nodeRead (read.c:338)
==00:00:00:02.155 3419654==    by 0x434879: nodeRead (read.c:452)
==00:00:00:02.155 3419654==    by 0x440E6C: _readTypeName (readfuncs.funcs.c:830)
==00:00:00:02.155 3419654==    by 0x44CC3A: parseNodeString (readfuncs.switch.c:121)
==00:00:00:02.155 3419654==    by 0x4348D6: nodeRead (read.c:338)
==00:00:00:02.155 3419654==    by 0x43D51D: _readFunctionParameter (readfuncs.funcs.c:2513)
==00:00:00:02.155 3419654==    by 0x44DE0C: parseNodeString (readfuncs.switch.c:367)
==00:00:00:02.155 3419654==    by 0x4348D6: nodeRead (read.c:338)
==00:00:00:02.155 3419654==    by 0x434879: nodeRead (read.c:452)
==00:00:00:02.155 3419654==    by 0x438A9C: _readCreateFunctionStmt (readfuncs.funcs.c:2499)
==00:00:00:02.155 3419654==  Address 0xf12f718 is 0 bytes inside a block of size 8 client-defined
==00:00:00:02.155 3419654==    at 0x6A70C3: MemoryContextAllocZeroAligned (mcxt.c:1109)
==00:00:00:02.155 3419654==    by 0x450C31: makeInteger (value.c:25)
==00:00:00:02.155 3419654==    by 0x434D59: nodeRead (read.c:482)
==00:00:00:02.155 3419654==    by 0x448690: _readA_Const (readfuncs.c:313)
==00:00:00:02.155 3419654==    by 0x44CCD2: parseNodeString (readfuncs.switch.c:129)
==00:00:00:02.155 3419654==    by 0x4348D6: nodeRead (read.c:338)
==00:00:00:02.155 3419654==    by 0x434879: nodeRead (read.c:452)
==00:00:00:02.155 3419654==    by 0x440E6C: _readTypeName (readfuncs.funcs.c:830)
==00:00:00:02.155 3419654==    by 0x44CC3A: parseNodeString (readfuncs.switch.c:121)
==00:00:00:02.155 3419654==    by 0x4348D6: nodeRead (read.c:338)
==00:00:00:02.155 3419654==    by 0x43D51D: _readFunctionParameter (readfuncs.funcs.c:2513)
==00:00:00:02.155 3419654==    by 0x44DE0C: parseNodeString (readfuncs.switch.c:367)
==00:00:00:02.155 3419654==

Here _readA_Const() performs:
                union ValUnion *tmp = nodeRead(NULL, 0);

                memcpy(&local_node->val, tmp, sizeof(*tmp));

where sizeof(union ValUnion) = 16, but nodeRead()->makeInteger() produced Integer (sizeof(Integer) = 8).

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:

Please look at the function _readA_Const() (introduced in a6bc33019), which fails on current master under valgrind:
...
Here _readA_Const() performs:
                union ValUnion *tmp = nodeRead(NULL, 0);

                memcpy(&local_node->val, tmp, sizeof(*tmp));

where sizeof(union ValUnion) = 16, but nodeRead()->makeInteger() produced Integer (sizeof(Integer) = 8).

Right, so we can't get away without a switch-on-value-type like the
other functions for A_Const have. Will fix.

regards, tom lane