Thread: pl/pgsql incorrect syntax checking on select .... into ... ?

Missed comma in select ... into construction don't raise ERROR both on
compiling and run time.


create table temp_table(i1 int, i2 int, i3 int, i4 int);
insert into temp_table(i1,i2,i3,i4) values (1,2,3,4);

CREATE OR REPLACE FUNCTION test_select_inti()
  RETURNS bool AS
$$
declare
  var_i1 int;
  var_i2 int;
  var_i3 int;
  var_i4 int;
begin

  --                  ---- missed comma \/ ----
  select i1,i2, i3, i4 into var_i1,var_i2 var_i3, var_i4
  from temp_table;

  raise notice '%, %, %, %',var_i1,var_i2,var_i3,var_i4;

  return true;
end
$$ LANGUAGE plpgsql VOLATILE SECURITY DEFINER;

select  test_select_inti();

#NOTICE:  1, 2, <NULL>, <NULL>

Artiom Makarov <artiom.makarov@gmail.com> writes:
> Missed comma in select ... into construction don't raise ERROR both on
> compiling and run time.

Unfortunately, that's perfectly legal syntax :-(.

The first problem is that long ago, somebody failed to make up their mind
about where the INTO clause could go in a plpgsql SELECT INTO, and
thought it would be cute to allow it anywhere.  What this means is
that the plpgsql parser gobbles as much as looks like a syntactically valid
variable list after INTO, and removes that from the statement, and then
sees if the main SQL parser likes what's left.  So starting with

>   --                  ---- missed comma \/ ----
>   select i1,i2, i3, i4 into var_i1,var_i2 var_i3, var_i4 from temp_table;

we remove "into var_i1,var_i2":

    select i1,i2, i3, i4 var_i3, var_i4 from temp_table;

The second problem is that that's valid SQL syntax, because even longer
ago, somebody thought it'd be a good idea to allow AS to be omitted before
SELECT output aliases.  So "var_i3" is a column alias to be applied to i4,
and then var_i4 is just a constant reference so far as this SELECT is
concerned.

The third problem is that SELECT INTO doesn't complain about extra output
columns; otherwise we might hope to at least get a complaint about that.
According to the comments in the source, this is necessary because
exec_move_row is sometimes applied to tuples from inheritance child tables
that might have extra physical columns.  I'm not really sure that's still
true, but even if it's not, people might not appreciate it if we started
throwing an error for code that used to be accepted.

In short: we could only make this throw an error if we were willing to
break some subset of existing valid (more or less) queries.

Personally I've always thought that "INTO anywhere" was a pretty darn bad
idea, but it's probably much too late to change that.

            regards, tom lane