Thread: pl/python invalidate functions with composite arguments
Here's a patch implementing properly invalidating functions that have composite type arguments after the type changes, as mentioned in http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's an incremental patch on top of the plpython-refactor patch sent eariler. Git branch for this patch: https://github.com/wulczer/postgres/tree/invalidate-composites. The idea in general is for this to work (taken straight from the unit tests, btw) CREATE TABLE employee ( name text, basesalary integer, bonus integer ); INSERT INTO employee VALUES ('John', 100, 10), ('Mary', 200, 10); CREATE OR REPLACE FUNCTION test_composite_table_input(e employee) RETURNS integer AS $$ return e['basesalary'] + e['bonus'] $$ LANGUAGE plpythonu; SELECT name, test_composite_table_input(employee.*) FROM employee; ALTER TABLE employee DROP bonus; -- this fails SELECT name, test_composite_table_input(employee.*) FROM employee; ALTER TABLE employee ADD bonus integer; UPDATE employee SET bonus = 10; -- this works again SELECT name, test_composite_table_input(employee.*) FROM employee; It's a long-standing TODO item, and a generally good thing to do. Cheers, Jan
Attachment
On 23/12/10 14:50, Jan Urbański wrote: > Here's a patch implementing properly invalidating functions that have > composite type arguments after the type changes, as mentioned in > http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's > an incremental patch on top of the plpython-refactor patch sent eariler. Updated to master.
Attachment
On 27/01/11 22:42, Jan Urbański wrote: > On 23/12/10 14:50, Jan Urbański wrote: >> Here's a patch implementing properly invalidating functions that have >> composite type arguments after the type changes, as mentioned in >> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's >> an incremental patch on top of the plpython-refactor patch sent eariler. > > Updated to master. Again.
Attachment
On Wed, Feb 9, 2011 at 02:09, Jan Urbański <wulczer@wulczer.org> wrote: > On 27/01/11 22:42, Jan Urbański wrote: >> On 23/12/10 14:50, Jan Urbański wrote: >>> Here's a patch implementing properly invalidating functions that have >>> composite type arguments after the type changes, as mentioned in >>> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's >>> an incremental patch on top of the plpython-refactor patch sent eariler. >> >> Updated to master. > > Again. Looks good, it works as described, the code is clean and well documented and it passes the added regression tests. I took the liberty of looking at the other pls to see how they handled this to find they don't cache them in the first place. For fun, I hacked plpython to not cache to see if there was any performance difference pre patch, post patch and in the non-cached cases. I couldn't find any probably due to: 1) my simple test case (select count(test_composite_table_input('(John, 100, "(10)")')) FROM generate_series(1, 1000000);) 2) things being cached 3) cache invalidation having to do most of the work that the non caching cache does. I think there is one or two more SearchSysCall's. 4) overhead from cassert It seems a bit heavy handed to invalidate and remake the entire plpython function whenever we hit this case. I think we could get away with setting ->is_rowtype = 2 in PLy_procedure_valid() instead. I suppose it should be fairly rare case anyway so... *shrug*.
On Thu, Feb 10, 2011 at 9:06 PM, Alex Hunsaker <badalex@gmail.com> wrote: > On Wed, Feb 9, 2011 at 02:09, Jan Urbański <wulczer@wulczer.org> wrote: >> On 27/01/11 22:42, Jan Urbański wrote: >>> On 23/12/10 14:50, Jan Urbański wrote: >>>> Here's a patch implementing properly invalidating functions that have >>>> composite type arguments after the type changes, as mentioned in >>>> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's >>>> an incremental patch on top of the plpython-refactor patch sent eariler. >>> >>> Updated to master. >> >> Again. > > Looks good, it works as described, the code is clean and well > documented and it passes the added regression tests. > > I took the liberty of looking at the other pls to see how they handled > this to find they don't cache them in the first place. For fun, I > hacked plpython to not cache to see if there was any performance > difference pre patch, post patch and in the non-cached cases. I > couldn't find any probably due to: > 1) my simple test case (select > count(test_composite_table_input('(John, 100, "(10)")')) FROM > generate_series(1, 1000000);) > 2) things being cached > 3) cache invalidation having to do most of the work that the non > caching cache does. I think there is one or two more SearchSysCall's. > 4) overhead from cassert > > It seems a bit heavy handed to invalidate and remake the entire > plpython function whenever we hit this case. I think we could get away > with setting ->is_rowtype = 2 in PLy_procedure_valid() instead. I > suppose it should be fairly rare case anyway so... *shrug*. This last comment might make me think that we're looking for a new version of this patch, but the comment on the CommitFest application says "looks good". So I'm not sure whether this should be marked Waiting on Author or Ready for Committer, but the current status of Needs Review looks wrong. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 11/02/11 16:47, Robert Haas wrote: > On Thu, Feb 10, 2011 at 9:06 PM, Alex Hunsaker <badalex@gmail.com> wrote: >> On Wed, Feb 9, 2011 at 02:09, Jan Urbański <wulczer@wulczer.org> wrote: >> It seems a bit heavy handed to invalidate and remake the entire >> plpython function whenever we hit this case. I think we could get away >> with setting ->is_rowtype = 2 in PLy_procedure_valid() instead. I >> suppose it should be fairly rare case anyway so... *shrug*. > > This last comment might make me think that we're looking for a new > version of this patch, but the comment on the CommitFest application > says "looks good". So I'm not sure whether this should be marked > Waiting on Author or Ready for Committer, but the current status of > Needs Review looks wrong. I'm not planning on writing a new version of the patch. AIUI Alex said, that there's a possible optimization to be done, but the gain is so small that it doesn't matter. Jan
On Fri, Feb 11, 2011 at 10:51 AM, Jan Urbański <wulczer@wulczer.org> wrote: > On 11/02/11 16:47, Robert Haas wrote: >> On Thu, Feb 10, 2011 at 9:06 PM, Alex Hunsaker <badalex@gmail.com> wrote: >>> On Wed, Feb 9, 2011 at 02:09, Jan Urbański <wulczer@wulczer.org> wrote: >>> It seems a bit heavy handed to invalidate and remake the entire >>> plpython function whenever we hit this case. I think we could get away >>> with setting ->is_rowtype = 2 in PLy_procedure_valid() instead. I >>> suppose it should be fairly rare case anyway so... *shrug*. >> >> This last comment might make me think that we're looking for a new >> version of this patch, but the comment on the CommitFest application >> says "looks good". So I'm not sure whether this should be marked >> Waiting on Author or Ready for Committer, but the current status of >> Needs Review looks wrong. > > I'm not planning on writing a new version of the patch. AIUI Alex said, > that there's a possible optimization to be done, but the gain is so > small that it doesn't matter. OK, marked Ready for Committer. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Feb 11, 2011 at 08:47, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Feb 10, 2011 at 9:06 PM, Alex Hunsaker <badalex@gmail.com> wrote: >> On Wed, Feb 9, 2011 at 02:09, Jan Urbański <wulczer@wulczer.org> wrote: >>> On 27/01/11 22:42, Jan Urbański wrote: >>>> On 23/12/10 14:50, Jan Urbański wrote: >>>>> Here's a patch implementing properly invalidating functions that have >>>>> composite type arguments after the type changes, as mentioned in >>>>> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's >>>>> an incremental patch on top of the plpython-refactor patch sent eariler. >>>> >>>> Updated to master. >>> >>> Again. >> >> Looks good, it works as described, the code is clean and well >> documented and it passes the added regression tests. >> >> I took the liberty of looking at the other pls to see how they handled >> this to find they don't cache them in the first place. For fun, I >> hacked plpython to not cache to see if there was any performance >> difference pre patch, post patch and in the non-cached cases. I >> couldn't find any probably due to: >> 1) my simple test case (select >> count(test_composite_table_input('(John, 100, "(10)")')) FROM >> generate_series(1, 1000000);) >> 2) things being cached >> 3) cache invalidation having to do most of the work that the non >> caching cache does. I think there is one or two more SearchSysCall's. >> 4) overhead from cassert >> >> It seems a bit heavy handed to invalidate and remake the entire >> plpython function whenever we hit this case. I think we could get away >> with setting ->is_rowtype = 2 in PLy_procedure_valid() instead. I >> suppose it should be fairly rare case anyway so... *shrug*. > > This last comment might make me think that we're looking for a new > version of this patch, but the comment on the CommitFest application > says "looks good". So I'm not sure whether this should be marked > Waiting on Author or Ready for Committer, but the current status of > Needs Review looks wrong. Drat, Ive been found it. I just didn't want to make things to easy for you. :)
On ons, 2011-02-09 at 10:09 +0100, Jan Urbański wrote: > On 27/01/11 22:42, Jan Urbański wrote: > > On 23/12/10 14:50, Jan Urbański wrote: > >> Here's a patch implementing properly invalidating functions that have > >> composite type arguments after the type changes, as mentioned in > >> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's > >> an incremental patch on top of the plpython-refactor patch sent eariler. > > > > Updated to master. > > Again. Committed.