Re: [PATCH] Add pretty-printed XML output option

On 22.02.23 08:20, Peter Smith wrote:
> Here are some review comments for patch v15-0001
>
> FYI, the patch applies clean and tests OK for me.
>
> ======
> doc/src/sgml/datatype.sgml
>
> 1.
> XMLSERIALIZE ( { DOCUMENT | CONTENT } <replaceable>value</replaceable>
> AS <replaceable>type</replaceable> [ { NO INDENT | INDENT } ] )
>
> ~
>
> Another/shorter way to write that syntax is like below. For me, it is
> easier to read. YMMV.
>
> XMLSERIALIZE ( { DOCUMENT | CONTENT } <replaceable>value</replaceable>
> AS <replaceable>type</replaceable> [ [NO] INDENT ] )
Indeed simpler to read.
> ======
> src/backend/executor/execExprInterp.c
>
> 2. ExecEvalXmlExpr
>
> @@ -3829,7 +3829,8 @@ ExecEvalXmlExpr(ExprState *state, ExprEvalStep *op)
>    {
>    Datum    *argvalue = op->d.xmlexpr.argvalue;
>    bool    *argnull = op->d.xmlexpr.argnull;
> -
> + bool    indent = op->d.xmlexpr.xexpr->indent;
> + text    *data;
>    /* argument type is known to be xml */
>    Assert(list_length(xexpr->args) == 1);
> Missing whitespace after the variable declarations
Whitespace added.
> ~~~
>
> 3.
> +
> + data = xmltotext_with_xmloption(DatumGetXmlP(value),
> + xexpr->xmloption);
> + if(indent)
> + *op->resvalue = PointerGetDatum(xmlformat(data));
> + else
> + *op->resvalue = PointerGetDatum(data);
> +
>    }
>
> Unnecessary blank line at the end.
blank line removed.
> ======
> src/backend/utils/adt/xml.
>
> 4. xmlformat
>
> +#else
> + NO_XML_SUPPORT();
> +return 0;
> +#endif
>
> Wrong indentation (return 0) in the indentation function? ;-)

indentation corrected.

v16 attached.

Thanks a lot!

Jim