On Mon, Mar 07, 2022 at 05:09:19PM -0500, Melanie Plageman wrote:
> I've attached a patch using the helper in most locations in contrib
> modules that seemed useful.
Thanks for the patch. I was also looking at that yesterday, and this
pretty much maps to what I have finished with, except for dblink and
xml2 where it was not looking that obvious to me at quick glance.
In xml2, my eyes hurt a bit on the meaning of doing the "Switch out of
SPI context" in xpath_table() after doing the two SPI calls, but I
agree that this extra switch back to the old context should not be
necessary. For dblink, I did not notice that the change for
dblink_get_notify() would be this straight-forward, good catch. It
would be nice to see prepTuplestoreResult() gone at the end, though I
am not sure how invasive/worth it would be for dblink/.
> - pg_logdir_ls() in contrib/adminpack has return type TYPEFUNC_RECORD
> and expectedDesc is not already created, so the helper can't be used.
> But basically, since it doesn't specify OUT argument names, it has to
> do TupleDescInitEntry() itself anyway, I think.
Here, actually, I thought first that a simplification should be
possible, noticing that the regression tests passed with the function
called until I saw what it was testing :)
I am fine to not poke at the beast, and it looks like we may run into
trouble if we wish to maintain compatibility down to adminpack 1.0 at
runtime. This function has a very limited usage, anyway.
> - contrib/tablefunc.c was also not simple to refactor. the various parts
> of SetSingleFuncCall are spread throughout different functions in the
> file.
>
> - contrib/dblink has one function which returns a tuplestore that was
> simple to change (dblink_get_notify()) and I've done so.
This matches my impression, so applied.
--
Michael
