Обсуждение: Re: pgsql: Remove absolete function TupleDescGetSlot().

Hi Andres,

On Tue, Sep 25, 2018 at 11:39:05PM +0000, Andres Freund wrote:
> Remove absolete function TupleDescGetSlot().
>
> TupleDescGetSlot() was kept around for backward compatibility for
> user-written SRFs. With the TupleTableSlot abstraction work, that code
> will need to be version specific anyway, so there's no point in
> keeping the function around any longer.

There are still references in the code to this function, and a
declaration of it:
git grep TupleDescGetSlot
doc/src/sgml/xfunc.sgml:     * user-defined SRFs that use the deprecated TupleDescGetSlot().
src/include/funcapi.h:   * user-defined SRFs that use the deprecated TupleDescGetSlot().
src/include/funcapi.h: * TupleTableSlot *TupleDescGetSlot(TupleDesc tupdesc) - Builds a
src/include/funcapi.h:extern TupleTableSlot *TupleDescGetSlot(TupleDesc tupdesc);
--
Michael

On 2018-09-26 09:04:14 +0900, Michael Paquier wrote:
> Hi Andres,
> 
> On Tue, Sep 25, 2018 at 11:39:05PM +0000, Andres Freund wrote:
> > Remove absolete function TupleDescGetSlot().
> > 
> > TupleDescGetSlot() was kept around for backward compatibility for
> > user-written SRFs. With the TupleTableSlot abstraction work, that code
> > will need to be version specific anyway, so there's no point in
> > keeping the function around any longer.
> 
> There are still references in the code to this function, and a
> declaration of it:

Hrmpf :/. Thanks for catching.


> src/include/funcapi.h: * TupleTableSlot *TupleDescGetSlot(TupleDesc tupdesc) - Builds a
> src/include/funcapi.h:extern TupleTableSlot *TupleDescGetSlot(TupleDesc tupdesc);

These two clearly need to go.


> git grep TupleDescGetSlot
> doc/src/sgml/xfunc.sgml:     * user-defined SRFs that use the deprecated TupleDescGetSlot().
> src/include/funcapi.h:   * user-defined SRFs that use the deprecated TupleDescGetSlot().

But here I'm less convinced. It's not entirely clear to me that the only
real reason for this to exists actually was TupleDescGetSlot(). OTOH, I
can't really see any proper reason to have it either.

Greetings,

Andres Freund

On Tue, Sep 25, 2018 at 05:10:39PM -0700, Andres Freund wrote:
>> git grep TupleDescGetSlot
>> doc/src/sgml/xfunc.sgml:     * user-defined SRFs that use the deprecated TupleDescGetSlot().
>> src/include/funcapi.h:   * user-defined SRFs that use the deprecated TupleDescGetSlot().
>
> But here I'm less convinced. It's not entirely clear to me that the only
> real reason for this to exists actually was TupleDescGetSlot(). OTOH, I
> can't really see any proper reason to have it either.

I have not been following the recent thread about the refactoring of
TupleSlot and such very closely, but if you don't plan to use
TupleTableSlot in FuncCallContext in the future, cannot this just go
away?  The function is not here anymore, so my take would be to get rid
of all things which relied on its presence.
--
Michael

On 2018-09-26 10:38:51 +0900, Michael Paquier wrote:
> On Tue, Sep 25, 2018 at 05:10:39PM -0700, Andres Freund wrote:
> >> git grep TupleDescGetSlot
> >> doc/src/sgml/xfunc.sgml:     * user-defined SRFs that use the deprecated TupleDescGetSlot().
> >> src/include/funcapi.h:   * user-defined SRFs that use the deprecated TupleDescGetSlot().
> > 
> > But here I'm less convinced. It's not entirely clear to me that the only
> > real reason for this to exists actually was TupleDescGetSlot(). OTOH, I
> > can't really see any proper reason to have it either.
> 
> I have not been following the recent thread about the refactoring of
> TupleSlot and such very closely, but if you don't plan to use
> TupleTableSlot in FuncCallContext in the future, cannot this just go
> away?  The function is not here anymore, so my take would be to get rid
> of all things which relied on its presence.

My point is that FuncCallContext->slot isn't actually all that related
to TupleDescGetSlot() and could be used entirely independently.

Greetings,

Andres Freund

On Tue, Sep 25, 2018 at 06:42:51PM -0700, Andres Freund wrote:
> My point is that FuncCallContext->slot isn't actually all that related
> to TupleDescGetSlot() and could be used entirely independently.

That's fair.  Why not just replacing the existing comment with something
like "slot can be used in your own context to store tuple values,
useful in the context of user-defined SRFs"?  Just my 2c.
--
Michael

Hi,

On 2018-09-26 12:41:38 +0900, Michael Paquier wrote:
> On Tue, Sep 25, 2018 at 06:42:51PM -0700, Andres Freund wrote:
> > My point is that FuncCallContext->slot isn't actually all that related
> > to TupleDescGetSlot() and could be used entirely independently.
> 
> That's fair.  Why not just replacing the existing comment with something
> like "slot can be used in your own context to store tuple values,
> useful in the context of user-defined SRFs"?  Just my 2c.

I've tried to do search for users of FuncCallContext->slot and couldn't
find anything. Therefore I'm more inclined to drop it too - just about
all cases where it's useful seem to require a more extensive
datastructure around anyway.  Unless somebody protests I'm going to that
in a few - if somebody later in the cycle signals a need for it, we can
still revert that part.

Greetings,

Andres Freund

On 2018-09-26 12:35:25 -0700, Andres Freund wrote:
> Hi,
> 
> On 2018-09-26 12:41:38 +0900, Michael Paquier wrote:
> > On Tue, Sep 25, 2018 at 06:42:51PM -0700, Andres Freund wrote:
> > > My point is that FuncCallContext->slot isn't actually all that related
> > > to TupleDescGetSlot() and could be used entirely independently.
> > 
> > That's fair.  Why not just replacing the existing comment with something
> > like "slot can be used in your own context to store tuple values,
> > useful in the context of user-defined SRFs"?  Just my 2c.
> 
> I've tried to do search for users of FuncCallContext->slot and couldn't
> find anything. Therefore I'm more inclined to drop it too - just about
> all cases where it's useful seem to require a more extensive
> datastructure around anyway.  Unless somebody protests I'm going to that
> in a few - if somebody later in the cycle signals a need for it, we can
> still revert that part.

Done so now.

Greetings,

Andres Freund