Re: pl/python invalidate functions with composite arguments

On Fri, Feb 11, 2011 at 08:47, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Feb 10, 2011 at 9:06 PM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>> On Wed, Feb 9, 2011 at 02:09, Jan Urbański <wulczer(at)wulczer(dot)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. :)