Re: [HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
Dean Rasheed dean.a.rash...@googlemail.com writes: 2010/1/9 Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
2010/1/10 Tom Lane t...@sss.pgh.pa.us: I've applied a patch along these lines. Cool. Thanks, that works great. Cheers, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
2009/12/4 Dean Rasheed dean.a.rash...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
Dean Rasheed dean.a.rash...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
2010/1/9 Tom Lane t...@sss.pgh.pa.us:
Dean Rasheed dean.a.rash...@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', 1) from generate_series(1,10);
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
Dean Rasheed dean.a.rash...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
Dean Rasheed dean.a.rash...@googlemail.com writes: 2010/1/9 Tom Lane t...@sss.pgh.pa.us: Dean Rasheed dean.a.rash...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
2010/1/9 Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: CVS HEAD: Error accessing system column from plpgsql trigger function
2010/1/9 Tom Lane t...@sss.pgh.pa.us: Dean Rasheed dean.a.rash...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
