Re: make tuplestore helper function

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Melanie Plageman <melanieplageman(at)gmail(dot)com>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Subject: Re: make tuplestore helper function
Date: 2022-03-08 01:17:03
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message 2022-03-08 01:25:13 RE: Logical replication timeout problem
Previous Message Tom Lane 2022-03-08 01:11:52 Re: Time to drop plpython2?