Re: *_to_xml() should copy SPI_processed/SPI_tuptable

Chapman Flack <chap(at)anastigmatix(dot)net> writes:
> In xml.c, query_to_xml_internal() contains a loop that refers
> to SPI_processed every iteration:
> for (i = 0; i < SPI_processed; i++)
> SPI_sql_row_to_xmlelement(i, result, tablename, nulls,
> tableforest, targetns, top_level);
> likewise, that function it is calling contains a loop that
> repeatedly dereferences SPI_tuptable:
> for (i = 1; i <= SPI_tuptable->tupdesc->natts; i++)
> ...
> ... map_sql_value_to_xml_value(colval,
> SPI_gettypeid(SPI_tuptable->tupdesc, ...
> and map_sql_value_to_xml_value can use OidOutputFunctionCall
> [ which could call arbitrary code that clobbers SPI_tuptable ]

Ugh. I wonder how many other places have latent issues of the same kind.

> The essence of the problem here seems to be simply that these routines
> in xml.c are not making local-variable copies of SPI_processed and
> SPI_tuptable, as the docs for SPI_execute recommend.

I think that's blaming the messenger TBH. The *real* problem is the
damfool decision to use global variables for this in the first place.
Should we change that somehow?

Really the right API would have entailed something like

int SPI_execute(const char *src, bool read_only, long tcount,
SPITupleTable **tuptable, uint64 *processed);

... and I guess we'd have to think about SPI_lastoid too, so maybe
returning a struct would be smarter than a pointer-per-value.

So some possible alternatives are:

* Just do it. Probably breaks too much 3rd-party code to be very
popular, though.

* Invent SPI_execute_r() and so on with APIs as above, and deprecate
the old functions, but don't remove 'em right away. Grotty, and it
does little to fix any latent problems that may exist outside core.

* Replace SPI_tuptable et al with macros that access fields in the
current SPI stack level (similar to the way that, eg, errno works
on most modern platforms). This seems do-able, if a bit grotty.

None of this is very back-patchable, so I guess we need a one-off
backpatched change for query_to_xml_internal and anyplace else that
looks to be at serious risk.

regards, tom lane