* Greg Smith (g...@2ndquadrant.com) wrote: > On 7/18/13 7:57 PM, Karol Trzcionka wrote: > >Current head 4cbe3ac3e86790d05c569de4585e5075a62a9b41 -> patch applies > >correct (only change needed in parallel_schedule). > >However it fails on own regression tests (other tests pass). > > I got a rejected hunk in src/backend/nodes/nodeFuncs.c as well as > that parallel_schedule issue. Maybe you didn't get the nodeFuncs > change but didn't notice that? That might explain why the tests > didn't work for you either.
The nodeFuncs.c hunk seems likely to have been impacted by the patch I
committed today (WITH CHECK OPTION), so I doubt that was the issue.
> Attached is an updated patch where I tried to only fix the two small
> hunks of bit rot. I get "All 140 tests passed" here, on a Mac no
> less.
Thanks for updating the patch, I ran into the failed hunks too and
expected to have to deal with them. :)
> I did a brief code scan through the patch just to get a feel for how
> the feature is put together, and what you'd need to know for a
> deeper review.
That would be extremely helpful.. Wasn't there a wiki page about this
feature which might also help? Bigger question- is it correct for the
latest version of the patch?
> (I'm trying to get customer time approved to work on
> this a lot more) The code was easier to follow than I expected.
> The way it completely avoids even getting into the security label
> integration yet seems like a successful design partitioning. This
> isn't nearly as scary as the SEPostgres patches. There are some
> useful looking utility functions that dump information about what's
> going on too.
>
> The bulk of the complexity is how the feature modifies query nodes
> to restrict what rows come through them. Some familiarity with that
> part of the code is what you'd need to take on reviewing this in
> detail. That and a week of time to spend trudging through it. If
> anyone is looking for an educational challenge on query execution,
> marching through all of these changes to validate they work as
> expected would do that.
I'm hoping to find time this weekend to look into this patch myself, but
the weekend is also filling up with other activities, so we'll see.
Thanks!
Stephen
signature.asc
Description: Digital signature
