On 29 July 2015 at 03:45, Heikki Linnakangas <hlinn...@iki.fi> wrote:
> On 07/28/2015 04:14 AM, David Rowley wrote: > >> On 27 July 2015 at 20:11, Heikki Linnakangas <hlinn...@iki.fi> wrote: >> >> On 07/27/2015 08:34 AM, David Rowley wrote: >>> >>> In this function I also wasn't quite sure if it was with comparing both >>>> non-NULL INITCOND's here. I believe my code comments may slightly >>>> contradict what the code actually does, as the comments talk about them >>>> having to match, but the code just bails if any are non-NULL. The >>>> reason I >>>> didn't check them was because it seems inevitable that some duplicate >>>> work >>>> needs to be done when setting up the INITCOND. Perhaps it's worth it? >>>> >>> >>> It would be nice to handle non-NULL initconds. I think you'll have to >>> check that the input function isn't volatile. Or perhaps just call the >>> input function, and check that the resulting Datum is byte-per-byte >>> identical, although that might be awkward to do with the current code >>> structure. >>> >> >> I've not done anything with this. >> I'd not thought of an input function being volatile before, but I guess >> it's possible, which makes me a bit scared that we could be treading on >> ground we shouldn't be. I know it's more of an output function thing than >> an input function thing, but a GUC like extra_float_digits could cause >> problems here. >> > > Yeah, a volatile input function seems highly unlikely, but who knows. BTW, > we're also not checking if the transition or final functions are volatile. > But that was the same before this patch too. > > It sure would be nice to support the built-in float aggregates, so I took > a stab at this. I heavily restructured the code again, so that there are > now two separate steps. First, we check for any identical Aggrefs that > could be shared. If that fails, we proceed to the permission checks, look > up the transition function and build the initial datum. And then we call > another function that tries to find an existing, compatible per-trans > structure. I think this actually looks better than before, and checking for > identical init values is now easy. This does lose one optimization: if > there are two aggregates with identical transition functions and final > functions, they are not merged into a single per-Agg struct. They still > share the same per-Trans struct, though, and I think that's enough. > > How does the attached patch look to you? The comments still need some > cleanup, in particular, the explanations of the different scenarios don't > belong where they are anymore. > I've read over the patch and you've managed to implement the init value checking much more cleanly than I had imagined it to be. I like the 2 stage checking. Attached is a delta patched which is based on sharing_aggstate-heikki-2.patch to fix up the out-dated comments and also a few more test scenarios which test the sharing works with matching INITCOND and that it does not when they don't match. What do you think? > > BTW, the permission checks were not correct before. You cannot skip the > check on the transition function when you're sharing the per-trans state. > We check that the aggregate's owner has permission to execute the > transition function, and the previous aggregate whose state value we're > sharing might have different owner. oops, thank for noticing that and fixing. Regards David Rowley -- David Rowley http://www.2ndQuadrant.com/ <http://www.2ndquadrant.com/> PostgreSQL Development, 24x7 Support, Training & Services
sharing_aggstate-heikki-2_delta1.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
