Horton

Thread

Commits

  1. Avoid copying undefined data in _readA_Const().

  2. Enable WRITE_READ_PARSE_PLAN_TREES of rewritten utility statements

  3. Implement WRITE_READ_PARSE_PLAN_TREES for raw parse trees

  4. Don't lose precision for float fields of Nodes.

  5. Fix write/read of empty string fields in Nodes.

  6. Add read support for some missing raw parse nodes

  7. Fix reading of BitString nodes

  8. Fix reading of most-negative integer value nodes

  1. Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-09T22:20:26Z

    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.

* The only reason struct Query still needs a handwritten output
function is that special logic is needed to prevent trying to
print the utilityStatement field when it's a utility statement
we lack outfuncs support for.  Now it wouldn't be that hard
to get gen_node_support.pl to replicate that special logic,
and if we stick with the status-quo functionality then I think we
should do that so that we can get rid of the handwritten function.
But the other alternative is to provide outfuncs support for all
utility statements and drop the conditionality.

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.  The attached WIP patch does that,
and gives me these code sizes:

$ size outfuncs.o readfuncs.o
   text    data     bss     dec     hex filename
 139503       0       0  139503   220ef outfuncs.o
  69356       0       0   69356   10eec readfuncs.o

(The extra readfuncs space comes from not troubling over the
subsidiary node types such as AlterTableCmd.  We could run
around and mark those no_read, but I didn't bother yet.)

The support-suppression code in gen_node_support.pl was a crude
hack before, and this patch doesn't make it any less so.
If we go this way, it would be better to move the knowledge that
we're suppressing read functionality into the utility statement
node declarations.  We could just manually label them all
pg_node_attr(no_read), but what I'm kind of tempted to do is
invent a dummy abstract node type like Expr, and make all the
utility statements inherit from it:

typedef struct UtilityStmt
{
	pg_node_attr(abstract, no_read)

	NodeTag		type;
} UtilityStmt;

Thoughts?

			regards, tom lane

  2. Re: Extending outfuncs support to utility statements

    Andres Freund <andres@anarazel.de> — 2022-07-10T21:43:48Z

    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

  3. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-10T23:12:52Z

    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

  4. Re: Extending outfuncs support to utility statements

    Andres Freund <andres@anarazel.de> — 2022-07-11T00:15:25Z

    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

  5. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T00:28:44Z

    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

  6. Re: Extending outfuncs support to utility statements

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2022-07-11T01:57:07Z

    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

  7. Re: Extending outfuncs support to utility statements

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-11T13:56:44Z

    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.

  8. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T14:16:37Z

    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

  9. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-12T22:01:39Z

    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

  10. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-12T22:38:26Z

    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

  11. Re: Extending outfuncs support to utility statements

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-08-24T15:25:31Z

    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.

  12. Re: Extending outfuncs support to utility statements

    Andres Freund <andres@anarazel.de> — 2022-09-22T15:32:57Z

    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

  13. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-22T16:16:30Z

    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

  14. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-22T16:48:47Z

    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

  15. Re: Extending outfuncs support to utility statements

    Andres Freund <andres@anarazel.de> — 2022-09-22T17:01:03Z

    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.

  16. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-22T20:57:14Z

    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

  17. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-22T21:21:27Z

    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

  18. Re: Extending outfuncs support to utility statements

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-09-26T14:46:49Z

    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.

  19. Re: Extending outfuncs support to utility statements

    Alexander Law <exclusion@gmail.com> — 2023-03-19T12:00:01Z

    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

  20. Re: Extending outfuncs support to utility statements

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-19T18:22:10Z

    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