These are chat archives for ManageIQ/manageiq/performance

13th
Apr 2016
Keenan Brock
@kbrock
Apr 13 2016 12:42
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
I broke the inner_list value with the detect?
Matthew Draper
@matthewd
Apr 13 2016 12:57
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
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
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
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
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
so subtile - thanks
unless inner_lists
Matthew Draper
@matthewd
Apr 13 2016 13:02
:+1:
Keenan Brock
@kbrock
Apr 13 2016 13:03
I guess that line will go away anyway
Matthew Draper
@matthewd
Apr 13 2016 13:03
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
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
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
ok, so ["a", "b"] is not valid input
Matthew Draper
@matthewd
Apr 13 2016 13:08
It is, as an equivalent of [["a"], ["b"]]
Keenan Brock
@kbrock
Apr 13 2016 13:08
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
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
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
Separate
Keenan Brock
@kbrock
Apr 13 2016 13:11
but same pr - thanks
Matthew Draper
@matthewd
Apr 13 2016 13:11
Not necessarily worthwhile on its own as you're rewriting stuff anyway
Keenan Brock
@kbrock
Apr 13 2016 13:11
I dunno
I think I need that logic
Matthew Draper
@matthewd
Apr 13 2016 13:11
I only mention it because it might still be relevant when you do your thing
Keenan Brock
@kbrock
Apr 13 2016 13:12
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
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
add_column :taggables, :tag_ids, :integer, :multi => true # array
drop_column :taggables, :tag_id
Matthew Draper
@matthewd
Apr 13 2016 13:14
Trades one set of inconveniences for another
Keenan Brock
@kbrock
Apr 13 2016 13:14
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
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
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
Good, but requires custom indexing / custom operators
Keenan Brock
@kbrock
Apr 13 2016 13:17
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
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
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
I'm not yet convinced the much uglier query will perform noticably worse
Keenan Brock
@kbrock
Apr 13 2016 13:27
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
+1
Keenan Brock
@kbrock
Apr 13 2016 13:29
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
Definitely should be explored
Keenan Brock
@kbrock
Apr 13 2016 13:29
@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
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
thanks
Matthew Draper
@matthewd
Apr 13 2016 13:32
(though there's also the question of how smoothly said change moves through replication)
Keenan Brock
@kbrock
Apr 13 2016 13:33
lol
:car: :fire: :fire_engine: :hospital:
Keenan Brock
@kbrock
Apr 13 2016 13:39
that sql clause, if there are limits/offsets, not sure if that will work
Keenan Brock
@kbrock
Apr 13 2016 13:58
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
@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
@kbrock GROUP BY hosts.id might do better than the DISTINCT?
Keenan Brock
@kbrock
Apr 13 2016 18:05
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

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
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
<— 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
it is all the same problem
Jason Frey
@Fryguy
Apr 13 2016 18:25
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
no, because lower(name) is what needs to be in the group by
Jason Frey
@Fryguy
Apr 13 2016 18:26
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
you have to sort by a name and us tags
dont think people do that
Chris Arcand
@chrisarcand
Apr 13 2016 18:29
Can someone besides myself make a simple user+group with some filters…?
Keenan Brock
@kbrock
Apr 13 2016 18:29
lol
(that was an embarased laugh)
Jason Frey
@Fryguy
Apr 13 2016 18:30
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
:neutral_face::expressionless::neutral_face::expressionless::neutral_face:
Jason Frey
@Fryguy
Apr 13 2016 18:31
LOL
Chris Arcand
@chrisarcand
Apr 13 2016 18:31
:sob:
Jason Frey
@Fryguy
Apr 13 2016 18:31
:anguished: :open_mouth: :anguished: :open_mouth: :anguished: :open_mouth:
Chris Arcand
@chrisarcand
Apr 13 2016 18:32
“worried panting”?
Keenan Brock
@kbrock
Apr 13 2016 18:32
I'm extracting the spec now and will join bisect world
every 5 seconds I'm bundle updateing