Prev: [COMMITTERS] pgsql: Tidy up and refactor plperl.c.
Next: [HACKERS] Congrats Alvaro!
From: Dean Rasheed on 9 Jan 2010 05:09
2009/12/4 Dean Rasheed <dean.a.rasheed(a)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 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 9 Jan 2010 13:59
Dean Rasheed <dean.a.rasheed(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Dean Rasheed on 9 Jan 2010 14:34
2010/1/9 Tom Lane <tgl(a)sss.pgh.pa.us>:
> Dean Rasheed <dean.a.rasheed(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 9 Jan 2010 14:54
Dean Rasheed <dean.a.rasheed(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Dean Rasheed on 9 Jan 2010 15:09
2010/1/9 Tom Lane <tgl(a)sss.pgh.pa.us>:
> Dean Rasheed <dean.a.rasheed(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2
Prev: [COMMITTERS] pgsql: Tidy up and refactor plperl.c.
Next: [HACKERS] Congrats Alvaro!