Re: CVS HEAD: Error accessing system column from plpgsql trigger function

2009/12/4 Dean Rasheed <dean(dot)a(dot)rasheed(at)googlemail(dot)com>:
> With CVS HEAD...
>
> create table foo (a int);
>
> create or replace function foo_trig_fn() returns trigger as $$
> begin
>  raise notice 'In trigger: added %', new.ctid;
>  return new;
> end
> $$ language plpgsql;
>
> create trigger foo_trig after insert on foo
>  for each row execute procedure foo_trig_fn();
>
> insert into foo values(1);
>
> ERROR:  attribute number -1 exceeds number of columns 1
>

I started thinking about this again, and it does indeed seem to be the commit
http://archives.postgresql.org/pgsql-committers/2009-11/msg00035.php which
causes this. Specifically, the change

* Avoid unnecessary scanner-driven lookups of plpgsql variables in
places where it's not needed, which is actually most of the time;
we do not need it in DECLARE sections nor in text that is a SQL
query or expression.

So read_sql_construct() now disables plpgsql variable lookups in
plpgsql_parse_dblword(), and old.foo/new.foo are compiled into FieldSelect
nodes, where they used to be record field param nodes, which is a problem for
ExecEvalFieldSelect() if foo is a system attribute.

How much do you really save by avoiding the plpgsql variable lookups in this
case? Is this just trading compilation time for execution time? In theory the
new code will be slower to execute because ExecEvalFieldSelect() goes through
ExecEvalParam() to get (a copy of) the whole record in order to extract the
required field, whereas the old code just calls ExecEvalParam() with dtype of
PLPGSQL_DTYPE_RECFIELD to retrieve the field directly. So perhaps
plpgsql_parse_dblword() should always just do the variable lookups.

Thoughts?

Regards,
Dean