Re: missing assert in makeString

st 19. 2. 2025 v 19:05 odesílatel Andres Freund <andres(at)anarazel(dot)de> napsal:

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

Any mechanism that can help is welcome. Unfortunately, I miss knowledge
about C static analyzers.
This issue can be pretty messy for beginners - who try to write their own
first patches. The error is raised
far to the point where this problem was created, and it needs a special
test or special configuration setting.

Probably this issue is less often than before, because out functions are
generated automatically, but there
can be bad design. Another possibility is implementing correct support for
NULL there, but probably it needs
format change, and it can be an issue for pg_upgrade.

Regards

Pavel

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