These are chat archives for ManageIQ/manageiq/performance

13th
Apr 2016
Keenan Brock
@kbrock
Apr 13 2016 12:42 UTC
hey @matthewd or @fryguy any opinion on ManageIQ/manageiq#7847 - or should I just build it up for my next pr?
Keenan Brock
@kbrock
Apr 13 2016 12:57 UTC
I broke the inner_list value with the detect?
Matthew Draper
@matthewd
Apr 13 2016 12:57 UTC
I'm unclear on this change, though… is it a minor refactoring (presumably ahead of some other change?), or is it actually doing a thing right here? If the latter, I'm missing it in amongst the other changes
c
Keenan Brock
@kbrock
Apr 13 2016 12:57 UTC
I want to change that function to build 1 large query and not query the database for each set of tags
this whole PR does "nothing"
prep for merging find_tagged_with and find_tags_by_grouping and go 100% scopes
Matthew Draper
@matthewd
Apr 13 2016 12:58 UTC
Okay, cool. That's what I was reading in the diff, but not the impression I got from the PR description
Keenan Brock
@kbrock
Apr 13 2016 12:58 UTC
so in Rbac, that spec can be merged
thanks
changing
but you said that line was wrong. I still dont see it :(
Matthew Draper
@matthewd
Apr 13 2016 13:00 UTC
detect is going to return nil or an array (because that's what the condition tests): never true
Keenan Brock
@kbrock
Apr 13 2016 13:01 UTC
so subtile - thanks
unless inner_lists
Matthew Draper
@matthewd
Apr 13 2016 13:02 UTC
:+1:
Keenan Brock
@kbrock
Apr 13 2016 13:03 UTC
I guess that line will go away anyway
Matthew Draper
@matthewd
Apr 13 2016 13:03 UTC
I guess you're about to kill this anyway, but it seems a better take on the current optimization would be to partition list on that condition
Keenan Brock
@kbrock
Apr 13 2016 13:03 UTC
man, I wonder when I can get arrays into there
hmm
[["a"],["b"],["c", "c2"]] - I'd want 3 lists
yea
not sure if I can do this mentaly
["a", "a1"] ==> 1 list
[["a"],["b"]] is optimized to 1 list
not sure if that is accurate
think that is a bug
well, if they have "all" then that is good
Matthew Draper
@matthewd
Apr 13 2016 13:07 UTC
Logic seems sound to me
Given that the top level array is an 'all' list, and inner ones are 'any' lists
Keenan Brock
@kbrock
Apr 13 2016 13:07 UTC
ok, so ["a", "b"] is not valid input
Matthew Draper
@matthewd
Apr 13 2016 13:08 UTC
It is, as an equivalent of [["a"], ["b"]]
Keenan Brock
@kbrock
Apr 13 2016 13:08 UTC
heh - this is going to squash down very small
right, but they will never input a single layer there
it will always be @param list [Array<Array<String>>]
ok - thanks
# If array is flat (contains no inner arrays) we can let find_tagged_with
that is wrong
Matthew Draper
@matthewd
Apr 13 2016 13:10 UTC
inner_lists, fixed_conditions = list.partition { |item| .. }
return find_tagged_with(options.merge(:all => fixed_conditions)).find_tags_by_grouping(inner_lists, options) if fixed_conditions.any?
return self if inner_lists.empty?
Keenan Brock
@kbrock
Apr 13 2016 13:10 UTC
ugh, set logic, in clauses (or <@) - sql does this for us :(
yea - that is cool.
same commit - or separate one?
Matthew Draper
@matthewd
Apr 13 2016 13:11 UTC
Separate
Keenan Brock
@kbrock
Apr 13 2016 13:11 UTC
but same pr - thanks
Matthew Draper
@matthewd
Apr 13 2016 13:11 UTC
Not necessarily worthwhile on its own as you're rewriting stuff anyway
Keenan Brock
@kbrock
Apr 13 2016 13:11 UTC
I dunno
I think I need that logic
Matthew Draper
@matthewd
Apr 13 2016 13:11 UTC
I only mention it because it might still be relevant when you do your thing
Keenan Brock
@kbrock
Apr 13 2016 13:12 UTC
because we're taking these partitions and tacking them into the query
yea - same page. agreed. think it is great
AND it is before I add sql. so that is even better
I like making big changes up front
w/o the switch flipped (changing data structures)
ok, tangent
talk me off this wall:
Matthew Draper
@matthewd
Apr 13 2016 13:13 UTC
Basically, I'm positing that even with an 'any' present, there will ~likely be multiple 'only'/required tags… and this allows us to simplify the to-be-nasty query by not treating each of those as a list-of-one
Keenan Brock
@kbrock
Apr 13 2016 13:14 UTC
add_column :taggables, :tag_ids, :integer, :multi => true # array
drop_column :taggables, :tag_id
Matthew Draper
@matthewd
Apr 13 2016 13:14 UTC
Trades one set of inconveniences for another
Keenan Brock
@kbrock
Apr 13 2016 13:14 UTC
matthewd - got it - same page. agreed it is a good change. and especially now before sql mess is introduced (next pr)
Matthew Draper
@matthewd
Apr 13 2016 13:15 UTC
Structured or not, it's still denormalization
e.g., indexing for tag-based lookup gets less pleasant
Keenan Brock
@kbrock
Apr 13 2016 13:15 UTC
gin/gist?
not so good?
wait - I thought that was more normalization
just happen to be using a single row instread of multiple rows
Matthew Draper
@matthewd
Apr 13 2016 13:17 UTC
Good, but requires custom indexing / custom operators
Keenan Brock
@kbrock
Apr 13 2016 13:17 UTC
they are in arel
and as it is, I'm going to have to 100% this puppy (not sure if I can arel it)
and with that other one, we can do a single inner join to taggables
now we have to do multiple left outer joins
hmm. maybe multiple left inner joins.
ooh - we currently can't do multiple left joins, because we'd end up with dups. so we have to do sub select exist / counts
ok, I'll try and sit on that
probably can't get it in by friday anyway
Matthew Draper
@matthewd
Apr 13 2016 13:20 UTC
It's a fair place to start a conversation, but "will make this one query much easier" doesn't seem a terribly strong reason, on its own, for schema surgery IMO
Keenan Brock
@kbrock
Apr 13 2016 13:25 UTC
major schema surgery
I think the performance will be noticable
and I think this query is used a lot.
it is all tagging
so basically every page
but yes
thanks for sanity check
AND I did preface it with "talk me off this ledge" ;)
Matthew Draper
@matthewd
Apr 13 2016 13:26 UTC
I'm not yet convinced the much uglier query will perform noticably worse
Keenan Brock
@kbrock
Apr 13 2016 13:27 UTC
a single join and 2 set operators agains a single indexed column (and no more of these preloader to the tags table)
possible no longer preloader to taggings
vs unknown number of 2 table away habtm subquery queries with count(*) and exists
ok - I'll generate the first one
and then we can tackle the "optimized" version - for next release
Matthew Draper
@matthewd
Apr 13 2016 13:29 UTC
+1
Keenan Brock
@kbrock
Apr 13 2016 13:29 UTC
follow through the refactor to the end.
Don't start a second one in the middle
sandy metz:
Matthew Draper
@matthewd
Apr 13 2016 13:29 UTC
Definitely should be explored
Keenan Brock
@kbrock
Apr 13 2016 13:29 UTC
@matthewd are you being nice, or honest?
A) "looks like he really wants to explore this. um. ok, look into that. he'll figure out it is a waste" OR B) "hmm. maybe, I dunno. try it"
Matthew Draper
@matthewd
Apr 13 2016 13:31 UTC
Honest.. the "yet" was earnest. Very well might be gainful, just not convinced it's "obviously" better
Keenan Brock
@kbrock
Apr 13 2016 13:31 UTC
thanks
Matthew Draper
@matthewd
Apr 13 2016 13:32 UTC
(though there's also the question of how smoothly said change moves through replication)
Keenan Brock
@kbrock
Apr 13 2016 13:33 UTC
lol
:car: :fire: :fire_engine: :hospital:
Keenan Brock
@kbrock
Apr 13 2016 13:39 UTC
that sql clause, if there are limits/offsets, not sure if that will work
Keenan Brock
@kbrock
Apr 13 2016 13:58 UTC
thanks @matthewd I was not adventerous enough for that other change, but will see what I can do once the sql queries start getting thrown around
Keenan Brock
@kbrock
Apr 13 2016 17:36 UTC
@matthewd error on master:
SELECT DISTINCT "hosts".* FROM "hosts" INNER JOIN "taggings" ON "taggings"."taggable_id" = "hosts"."id" AND "taggings"."taggable_type" = $1 WHERE "taggings"."tag_id" IN (8298) ORDER BY lower(name)
I don't think we introduced, but it can't do the distinct (or a group by) because of the order by lower(name)
I dont think we introduced but.
suggestion to fix this or just plod forward and exterminate the stupid distinct / group by for the taggings?
Matthew Draper
@matthewd
Apr 13 2016 18:05 UTC
@kbrock GROUP BY hosts.id might do better than the DISTINCT?
Keenan Brock
@kbrock
Apr 13 2016 18:05 UTC
yea, I'm hoping today I'll have a pr to tear that junk out
ManageIQ/manageiq#7935
that is 95% disposable
but 500 on all screens for a few people
Jason Frey
@Fryguy
Apr 13 2016 18:19 UTC

for a few people

Do you know why it's only a few peiople? What are the conditions leading to it?

Matthew Draper
@matthewd
Apr 13 2016 18:20 UTC
Do said few people have a relatively old version of PostgreSQL? Or just specific local data/config?
Chris Arcand
@chrisarcand
Apr 13 2016 18:22 UTC
<— a few people
On fresh master, db, etc I just added a user to a group with some tag filters and logged in, get errors on most pages: https://gist.github.com/chrisarcand/115e08ba4cb6bf7d953200a8680dbd76
Gitter why do you even try to popup gists...
Keenan Brock
@kbrock
Apr 13 2016 18:24 UTC
it is all the same problem
Jason Frey
@Fryguy
Apr 13 2016 18:25 UTC
can you just do lower(name) AS name?
I mean as a tactical fix, would that even work?
Keenan Brock
@kbrock
Apr 13 2016 18:25 UTC
no, because lower(name) is what needs to be in the group by
Jason Frey
@Fryguy
Apr 13 2016 18:26 UTC
yeah, but AS name is in the select list (sort of :) )
so, back to my question...why don't others have this problem
and also what changed that this suddenly fails (I don't see any very recent changes to tag, tagging, ar_taggable)...to which git bisect might narrow that down
Keenan Brock
@kbrock
Apr 13 2016 18:29 UTC
you have to sort by a name and us tags
dont think people do that
Chris Arcand
@chrisarcand
Apr 13 2016 18:29 UTC
Can someone besides myself make a simple user+group with some filters…?
Keenan Brock
@kbrock
Apr 13 2016 18:29 UTC
lol
(that was an embarased laugh)
Jason Frey
@Fryguy
Apr 13 2016 18:30 UTC
So, is the thought that @chrisarcand has a presorted screen sorted by name that also has tags?
Chris Arcand
@chrisarcand
Apr 13 2016 18:30 UTC
:neutral_face::expressionless::neutral_face::expressionless::neutral_face:
Jason Frey
@Fryguy
Apr 13 2016 18:31 UTC
LOL
Chris Arcand
@chrisarcand
Apr 13 2016 18:31 UTC
:sob:
Jason Frey
@Fryguy
Apr 13 2016 18:31 UTC
:anguished: :open_mouth: :anguished: :open_mouth: :anguished: :open_mouth:
Chris Arcand
@chrisarcand
Apr 13 2016 18:32 UTC
“worried panting”?
Keenan Brock
@kbrock
Apr 13 2016 18:32 UTC
I'm extracting the spec now and will join bisect world
every 5 seconds I'm bundle updateing