Re: missing assert in makeString

Hi

st 19. 2. 2025 v 7:48 odesílatel Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> napsal:

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

Other makeFoo functions have arguments of Node * type, and there the NULL
is not problematic,
or has arguments of valuable types, or holds flag xxxisnull and handles
nulls correctly.

Similar is makeAlias(const char *aliasname, List *colnames),
makeColumnDef(...)

but these functions crashes early due usage of pstrdup

DefElem doesn't do pstrdup and it can crash too, so the assertions should
be there too.

Unfortunately, the original patch had not completed tests - there was
missing forcing an expression serialization. So it worked. Surprisingly it
fails on BSD, where expression serialization was forced (I didn't
investigate what is different).

I think makeFoo() should produce a result that doesn't fail anywhere (when
it was not modified badly later). So they should accept only data that we
correctly serialize and deserialize.

Moreover, it fails early. The out func crashes too late, and it needs
special regress tests. Unfortunately, there is not any flag that can
signal, so some tests are missing.

Regards

Pavel

> regards, tom lane
>