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

With CVS HEAD, I'm getting the following error when I try to access a
system column from within trigger (which I'm doing just for debug
purposes):

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

As far as I've been able to work out, this started after this commit:
http://archives.postgresql.org/pgsql-committers/2009-11/msg00035.php
but that's an area of the code I know nothing about. It looks like a
change to the parsing code is causing it to use a FieldSelect node for
the system attribute, where it didn't used to.
- Dean

2009/12/4 Dean Rasheed <dean.a.rasheed@googlemail.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
mostof 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

Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
>> ERROR: �attribute number -1 exceeds number of columns 1

I guess your previous message slipped through the cracks --- sorry about
that.  It looks like the best fix is to teach ExecEvalFieldSelect that
references to system columns are OK.  Working on it now.
        regards, tom lane

2010/1/9 Tom Lane <tgl@sss.pgh.pa.us>:
> Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
>>> ERROR:  attribute number -1 exceeds number of columns 1
>
> I guess your previous message slipped through the cracks --- sorry about
> that.  It looks like the best fix is to teach ExecEvalFieldSelect that
> references to system columns are OK.  Working on it now.
>

I wonder if it might be better to have plpgsql_parse_dblword() ignore
plpgsql_LookupIdentifiers, and always do the lookups. In addition to
fixing my original gripe, the resulting parse tree is simpler and slightly
faster to execute. Admittedly you have to work quite hard to contrive a
test case where the performance difference is noticeable, but with the
test code below patching plpgsql_parse_dblword() gives around a 4%
performance boost to the INSERT.

create table foo (val text, len int);

create or replace function foo_trig_fn() returns trigger as $$
begin new.len := length(new.val); return new;
end
$$ language plpgsql;

create trigger foo_trig before insert on foo for each row execute procedure foo_trig_fn();

insert into foo(val) select repeat('X', 100000000) from generate_series(1,10);

Regards,
Dean

Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
> I wonder if it might be better to have plpgsql_parse_dblword() ignore
> plpgsql_LookupIdentifiers, and always do the lookups.

Not if you'd like things to still work.
        regards, tom lane

2010/1/9 Tom Lane <tgl@sss.pgh.pa.us>:
> Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
>> I wonder if it might be better to have plpgsql_parse_dblword() ignore
>> plpgsql_LookupIdentifiers, and always do the lookups.
>
> Not if you'd like things to still work.
>

OK, I admit that I'm totally new that area of code, so I'm not seeing
it - what does it break?
[regression tests still pass BTW]

Regards,
Dean

Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
> 2010/1/9 Tom Lane <tgl@sss.pgh.pa.us>:
>> Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
>>> I wonder if it might be better to have plpgsql_parse_dblword() ignore
>>> plpgsql_LookupIdentifiers, and always do the lookups.

>> Not if you'd like things to still work.

> OK, I admit that I'm totally new that area of code, so I'm not seeing
> it - what does it break?

The main problem is it will throw errors in some cases where that's
premature.  For instance we might have a.x where a is a plpgsql
row variable that doesn't contain x ... but if the reference is
in a query where a is a table that contains x, and we are using
prefer-the-column rules, this is not an error case.  Also we do
not want any lookups while looking at DECLARE constructs ---
it doesn't matter whether there's an outer-scope variable of the
same name.

However, it turns out my solution isn't working too well either :-(.
I can make it work for some of the system columns, but not for xmin,
xmax, or cmin/cmax because those fields of a regular tuple are overlaid
with datum-tuple header fields.  So by the time ExecEvalFieldSelect gets
the tuple those values are irretrievably trashed.

I think that a variant of your idea could be made to work: change
plpgsql_LookupIdentifiers to a three-state variable (which'd basically
mean "in DECLARE, in a SQL expression, anywhere else"), do no lookups in
DECLARE, and in SQL expressions do lookups but never throw any errors.
I'll have a go at that.
        regards, tom lane

2010/1/9 Tom Lane <tgl@sss.pgh.pa.us>:
>> OK, I admit that I'm totally new that area of code, so I'm not seeing
>> it - what does it break?
>
> The main problem is...

Ah OK. Thanks for the explanation.

> I think that a variant of your idea could be made to work: change
> plpgsql_LookupIdentifiers to a three-state variable (which'd basically
> mean "in DECLARE, in a SQL expression, anywhere else"), do no lookups in
> DECLARE, and in SQL expressions do lookups but never throw any errors.
> I'll have a go at that.
>

Sounds plausible.

Cheers,
Dean

Dean Rasheed <dean.a.rasheed@googlemail.com> writes:
> 2010/1/9 Tom Lane <tgl@sss.pgh.pa.us>:
>> I think that a variant of your idea could be made to work: change
>> plpgsql_LookupIdentifiers to a three-state variable (which'd basically
>> mean "in DECLARE, in a SQL expression, anywhere else"), do no lookups in
>> DECLARE, and in SQL expressions do lookups but never throw any errors.
>> I'll have a go at that.

> Sounds plausible.

I've applied a patch along these lines.
        regards, tom lane

2010/1/10 Tom Lane <tgl@sss.pgh.pa.us>:
>
> I've applied a patch along these lines.
>

Cool. Thanks, that works great.

Cheers,
Dean