Обсуждение: Using scalar function as set-returning: bug or feature?

Hi hackers,

I wonder if the following behavior is considered to be a bug in plpgsql 
or it is expected result:

create table my_data(id serial primary key, time timestamp, type text);

create or replace function my_insert(type text) RETURNS BOOLEAN
AS
$BODY$
BEGIN
     insert into my_data (time, type) values (now(), type);
     return true;
END
$BODY$
LANGUAGE plpgsql;

create or replace function my_call_insert() RETURNS BOOLEAN
AS
$BODY$
DECLARE
   b boolean;
BEGIN
     select into b from my_insert('from func atx');
     return b;
END
$BODY$
LANGUAGE plpgsql;

select my_call_insert();
  my_call_insert
----------------

(1 row)

================================================

So, if function returning boolean is used in from list, then no error or 
warning is produced, but null value is assigned to the target variable.
If this code is rewritten in more natural way:

     b := my_insert('from func atx');
or
    select my_insert('from func atx') into b;

then function returns expected result (true).
I spent some time investigating this code under debugger and looks like 
the reason of the problem is that tuple descriptor for the row returned 
by my_insert() contains no columns (number of attributes is zero). May 
be there are some reasons for such behavior, but I find it quite 
confusing and unnatural. I prefer to report error in this case or return 
tuple with single column, containing return value of the function.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello

> select into b from my_insert('from func atx');
You missed select something into b. For example,
select ret into b from my_insert('from func atx') as ret;

Using scalar function in from is not bug.
Silent assigning NULL for variables in "into" not matches same in "select"... I think better would be raise warning.

regards, Sergei

On 09.02.2018 10:47, Sergei Kornilov wrote:
> Hello
>
>> select into b from my_insert('from func atx');
> You missed select something into b. For example,
> select ret into b from my_insert('from func atx') as ret;
>
> Using scalar function in from is not bug.
> Silent assigning NULL for variables in "into" not matches same in "select"... I think better would be raise warning.
>
> regards, Sergei
Thank you.
Really the problem is caused by empty source list for INTO.
If I rewrite query as

     select my_insert into b from my_insert('from func atx');

then it works as expected.
But I wonder if the original statement should be considered as error or 
at least we should produce warning for such empty projections?


-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 09.02.2018 11:02, Konstantin Knizhnik wrote:
>
>
> On 09.02.2018 10:47, Sergei Kornilov wrote:
>> Hello
>>
>>> select into b from my_insert('from func atx');
>> You missed select something into b. For example,
>> select ret into b from my_insert('from func atx') as ret;
>>
>> Using scalar function in from is not bug.
>> Silent assigning NULL for variables in "into" not matches same in 
>> "select"... I think better would be raise warning.
>>
>> regards, Sergei
> Thank you.
> Really the problem is caused by empty source list for INTO.
> If I rewrite query as
>
>     select my_insert into b from my_insert('from func atx');
>
> then it works as expected.
> But I wonder if the original statement should be considered as error 
> or at least we should produce warning for such empty projections?
>
>
Attached please find patch reporting error in case of empty attribute 
list for SELECT INTO.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 09.02.2018 11:02, Konstantin Knizhnik wrote:

On 09.02.2018 10:47, Sergei Kornilov wrote:

Hello

select into b from my_insert('from func atx');

You missed select something into b. For example,
select ret into b from my_insert('from func atx') as ret;

Using scalar function in from is not bug.
Silent assigning NULL for variables in "into" not matches same in "select"... I think better would be raise warning.

regards, Sergei

Thank you.
Really the problem is caused by empty source list for INTO.
If I rewrite query as

    select my_insert into b from my_insert('from func atx');

then it works as expected.
But I wonder if the original statement should be considered as error or at least we should produce warning for such empty projections?

Attached please find patch reporting error in case of empty attribute list for SELECT INTO.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2018-02-09 12:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>> This is quite short-sighted.  The better way to do this is to complain if
>> the number of expressions is different from the number of target variables
>> (and the target variable is not a record-ish type).  There's been at least
>> two patches for this earlier (one my me, and one by, I think Pavel
>> Stehule).  I urge you to dig around in the archives to avoid wasting your
>> time.

> This issue can be detected by plpgsql_check and commitfest pipe is patch
> that raise warning or error in this case.

I think the issue basically arises from this concern in exec_move_row:

     * Row is a bit more complicated in that we assign the individual
     * attributes of the tuple to the variables the row points to.
     *
     * NOTE: this code used to demand row->nfields ==
     * HeapTupleHeaderGetNatts(tup->t_data), but that's wrong.  The tuple
     * might have more fields than we expected if it's from an
     * inheritance-child table of the current table, or it might have fewer if
     * the table has had columns added by ALTER TABLE. Ignore extra columns
     * and assume NULL for missing columns, the same as heap_getattr would do.
     * We also have to skip over dropped columns in either the source or
     * destination.

As things stand today, we would have a hard time tightening that up
without producing unwanted complaints about the cases mentioned in
this comment, because the DTYPE_ROW logic is used for both "INTO a,b,c"
and composite-type variables.  However, my pending patch at
https://commitfest.postgresql.org/17/1439/
gets rid of the use of DTYPE_ROW for composite types, and once that
is in it might well be reasonable to just throw a flat-out error for
wrong number of source values for a DTYPE_ROW target.  I can't
immediately think of any good reason why you'd want to allow for
the number of INTO items not matching what the query produces.

            regards, tom lane

I wrote:
> I think the issue basically arises from this concern in exec_move_row:

>      * NOTE: this code used to demand row->nfields ==
>      * HeapTupleHeaderGetNatts(tup->t_data), but that's wrong.  The tuple
>      * might have more fields than we expected if it's from an
>      * inheritance-child table of the current table, or it might have fewer if
>      * the table has had columns added by ALTER TABLE.

BTW, this comment is very old, dating to commit 8bb3c8fe5, which was
in response to
https://www.postgresql.org/message-id/flat/200104300537.f3U5brK62902%40hub.org

Looking at it again now, I wonder whether I didn't make the wrong choice
about how to fix the problem.  The above concerns seem valid so far as the
physical number of fields in the tuple go, but they do not apply to the
tupdesc that comes with it.  So arguably the real bug was using the
physical tuple rather than the tupdesc to drive the conversion, which is
something we got away from later anyway when we added dropped-column
support.  Perhaps it'd be reasonable to throw an error if the number of
non-dropped columns in the source and destination tupdescs don't match,
even for the composite-type case.

The specific issue mentioned in the above bug report is gone anyway: if I
try that test case now, I don't see any tuples with extra columns arriving
at exec_move_row, because we insert a ConvertRowtypeExpr node to convert
the rows of the child table to the parent rowtype.  But I'm not quite
prepared to assume that such cases can't arise through other examples.

Now, I do not think we can remove the facility for doing data type
conversion on the fields; undoubtedly there are people relying on being
able to SELECT INTO a numeric variable from an integer source column, for
example.  So maybe being lax about the number of columns is a sensible
thing to go along with that.  But it sure seems like a foot-gun.

            regards, tom lane

Moin Tom,

On Mon, February 12, 2018 5:03 pm, Tom Lane wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> 2018-02-09 12:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>>> This is quite short-sighted.  The better way to do this is to complain
>>> if
>>> the number of expressions is different from the number of target
>>> variables
>>> (and the target variable is not a record-ish type).  There's been at
>>> least
>>> two patches for this earlier (one my me, and one by, I think Pavel
>>> Stehule).  I urge you to dig around in the archives to avoid wasting
>>> your
>>> time.
[snip a bit]

> As things stand today, we would have a hard time tightening that up
> without producing unwanted complaints about the cases mentioned in
> this comment, because the DTYPE_ROW logic is used for both "INTO a,b,c"
> and composite-type variables.  However, my pending patch at
> https://commitfest.postgresql.org/17/1439/
> gets rid of the use of DTYPE_ROW for composite types, and once that
> is in it might well be reasonable to just throw a flat-out error for
> wrong number of source values for a DTYPE_ROW target.  I can't
> immediately think of any good reason why you'd want to allow for
> the number of INTO items not matching what the query produces.

Perl lets you set a fixed number of multiple variables from an array and
discard the rest like so:

  my ($a, $b) = (1,2,3);

and the right hand side can also be a function:

  my ($c, $d) = somefunc( 123 );

Where somefunc() returns more than 2 params.

This is quite handy if you sometimes need more ore less return values, but
don't want to create almost-identical functions for each case.

I'm not sure if you mean exactly the scenario as in the attached test
case, but this works in plpgsql, too, and would be a shame to lose.

OTOH, one could also write:

  SELECT INTO ba, bb  a,b FROM foo(1);

and it would still work, or wouldn't it?

Best regards,

Tels

"Tels" <nospam-pg-abuse@bloodgate.com> writes:
> On Mon, February 12, 2018 5:03 pm, Tom Lane wrote:
>> ... However, my pending patch at
>> https://commitfest.postgresql.org/17/1439/
>> gets rid of the use of DTYPE_ROW for composite types, and once that
>> is in it might well be reasonable to just throw a flat-out error for
>> wrong number of source values for a DTYPE_ROW target.  I can't
>> immediately think of any good reason why you'd want to allow for
>> the number of INTO items not matching what the query produces.

> Perl lets you set a fixed number of multiple variables from an array and
> discard the rest like so:
>   my ($a, $b) = (1,2,3);
> I'm not sure if you mean exactly the scenario as in the attached test
> case, but this works in plpgsql, too, and would be a shame to lose.

Well, that's exactly the issue.  Whether that's a handy feature or
a foot-gun that hides bugs depends entirely on your point of view.
Personally, this is not the kind of behavior I really want from a
programming language ;-).  And I'm sure that if plpgsql were still
enforcing the old rules, and someone came along with a proposal to
relax that to be more like Perl, it'd be laughed down.

Still, backwards compatibility is worth something.  I don't really
have a strong opinion about whether to change it or not.

            regards, tom lane