Re: missing assert in makeString

Hi,

On 2025-02-19 01:48:53 -0500, Tom Lane wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> > I investigated the crashes in "xmlnamespaces to xmlelement" patch and it
> > looks like there was a badly used makeString function. The argument should
> > not be null, elsewhere serialization to string fails - and deserialization
> > doesn't support this case.
> > I propose to add an assert there like (make check-world passed)
>
> Hmmm ... while I don't necessarily object to this patch, we have a lot
> of makeFoo() functions that build nodes, and hardly any of them have
> asserts like this one.

I also suspect that adding is-not-NULL asserts isn't that helpful on its own,
because you still need to reach that function with it set to NULL. We probably
should use pg_attribute_nonnull() much more widely, so that compilers and
static analyzers can help.

> Why makeString() in particular? Is the fault on the serialization side,
> instead? If there's a general expectation that a String node's value isn't
> null, how come the original patch worked at all?

It's worth noting that the CI task just failed on freebsd, which builds with:

CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

PG_TEST_INITDB_EXTRA_OPTS: -c debug_copy_parse_plan_trees=on -c debug_write_read_parse_plan_trees=on -c debug_raw_expression_coverage_test=on

Greetings,

Andres Freund