Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Fix "explain" feature in the debug plugin #30580

Merged
merged 4 commits into from
Sep 9, 2020

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Sep 6, 2020

Pull Request for Issue #30318 .

Summary of Changes

Fixing query explain feature. This also update joomla/database lib.

Testing Instructions

Run composer install and npm install

Enable debug plugin and enable "Query explain" feature.
In the Debugbar, in Query tab you should be able to see Explain , near the every "select" query.
Click on it.

Actual result BEFORE applying this Pull Request

it show empty table.

Expected result AFTER applying this Pull Request

it show "explain"

Documentation Changes Required

none

@richard67
Copy link
Member

Setting release blocker label as inherited from the issue.

@richard67
Copy link
Member

@Fedik It works for me with MySQLi and MySQL (PDO), but it doesn't work here with PostgreSQL (PDO):

snap-4

@Fedik
Copy link
Member Author

Fedik commented Sep 6, 2020

hm, I suspect PostgreSQL has different "result format" for EXPLAIN query,
can you please try to add dump($this->explains[$k]); after try/catch block, around:

and make a screenshot of output for me?
I do not have Postgres installed, and cannot check on my own.

@richard67
Copy link
Member

It may take some time until I can do that.

@richard67
Copy link
Member

@Fedik The output is an empty string. But there is no exception. I am just reading https://www.postgresql.org/docs/12/using-explain.html . Maybe we have to use EXPLAIN ANALYZE and not just EXPLAIN with PostgreSQL? => Ping @alikon .

@Fedik
Copy link
Member Author

Fedik commented Sep 6, 2020

Maybe we have to use EXPLAIN ANALYZE and not just EXPLAIN with PostgreSQL?

tbh I have no idea ;)
here need someone who know PostgreSQL

Thanks for checking it

@richard67
Copy link
Member

@Fedik Sorry, my mistake, was not empty string, I was just too silly to find the output.

Here the output for one query:

array(2) { [0]=> array(1) { ["QUERY PLAN"]=> string(60) "Seq Scan on j4ux0_session (cost=0.00..1.01 rows=1 width=18)" } [1]=> array(1) { ["QUERY PLAN"]=> string(89) " Filter: (session_id = '\\x6c6e71646d70753034337136386b7562317534386a3537636873'::bytea)" } } array(2) { [0]=> array(1) { ["QUERY PLAN"]=> string(60) "Seq Scan on j4ux0_session (cost=0.00..1.01 rows=1 width=18)" } [1]=> array(1) { ["QUERY PLAN"]=> string(89) " Filter: (session_id = '\\x6c6e71646d70753034337136386b7562317534386a3537636873'::bytea)" } } array(2) { [0]=> array(1) { ["QUERY PLAN"]=> string(60) "Seq Scan on j4ux0_session (cost=0.00..1.01 rows=1 width=18)" } [1]=> array(1) { ["QUERY PLAN"]=> string(89) " Filter: (session_id = '\\x6c6e71646d70753034337136386b7562317534386a3537636873'::bytea)" } } 

Does that help?

@Fedik
Copy link
Member Author

Fedik commented Sep 6, 2020

yea, thanks! I will look what can do with it

@Fedik
Copy link
Member Author

Fedik commented Sep 6, 2020

I made some "blind changes", hope it work now 😄

@richard67
Copy link
Member

@Fedik Now it outputs something like this:


Explain
--
QUERY PLAN | Merge Left Join  (cost=28.76..44.29 rows=4 width=194)
QUERY PLAN | Merge Cond: (a.attnum = adef.adnum)
QUERY PLAN | Join Filter: (a.attrelid = adef.adrelid)
QUERY PLAN | InitPlan 2 (returns $1)
QUERY PLAN | ->  Index Scan using pg_class_relname_nsp_index on pg_class  (cost=1.35..9.37 rows=1 width=4)
QUERY PLAN | Index Cond: ((relname = 'j4ux0_users'::name) AND (relnamespace = $0))
QUERY PLAN | InitPlan 1 (returns $0)
QUERY PLAN | ->  Seq Scan on pg_namespace  (cost=0.00..1.07 rows=1 width=4)
QUERY PLAN | Filter: (nspname = 'public'::name)
QUERY PLAN | ->  Index Scan using pg_attribute_relid_attnum_index on pg_attribute a  (cost=0.28..13.71 rows=4 width=79)
QUERY PLAN | Index Cond: ((attrelid = $1) AND (attnum > 0))
QUERY PLAN | Filter: (NOT attisdropped)
QUERY PLAN | ->  Sort  (cost=19.10..19.12 rows=7 width=413)
QUERY PLAN | Sort Key: adef.adnum
QUERY PLAN | ->  Index Scan using pg_attrdef_adrelid_adnum_index on pg_attrdef adef  (cost=0.27..19.01 rows=7 width=413)
QUERY PLAN | Index Cond: (adrelid = $1)

I had to forced reload the page because the js was a bit sticky in browser cache.

Not sure if the above output is what is expected.

@alikon What do you think? Is it sufficient for you?

@Fedik
Copy link
Member Author

Fedik commented Sep 6, 2020

Now it outputs something like this

I see, I will try to change a rendering a bit, it enough only one "QUERY PLAN" :)

to something like:

QUERY PLAN
--------------------------
Merge Left Join  (cost=28.76..44.29 rows=4 width=194)
Merge Cond: (a.attnum = adef.adnum)
Join Filter: (a.attrelid = adef.adrelid)
InitPlan 2 (returns $1)
...
...

…es5.js

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@alikon
Copy link
Contributor

alikon commented Sep 7, 2020

from 3

Screenshot from 2020-09-07 18-27-01

from 4 + pr
Screenshot from 2020-09-07 18-29-00

so yes just 1"QUERY PLAN" 😄

@alikon
Copy link
Contributor

alikon commented Sep 7, 2020

guess maybe room for another pr

i've noticed that it stll shows the parameter marker :userid in the query
in the explain it use correctly the real value 413

should be showed in the sql query text too imho

@richard67
Copy link
Member

@alikon I think that was not subject of this PR to show the bind variables' value, and I am not even sure if it was the main subject of an issue or just a comment in the issue which was release blocker. Here just the empty explain plan is fixed.

But from my point of view exactly that is the release blocker, that we either need to replace the bind variables by their values, or if this is not possible at least show the values e.g. on side so we can fiddle out what the values in the query are.

Could you check if we have an issue for that and if not, make one?

@Fedik
Copy link
Member Author

Fedik commented Sep 7, 2020

i've noticed that it stll shows the parameter marker :userid in the query
should be showed in the sql query text too imho

it not possible with PDO, for this need to write a query parser, to rebuild whole query string,
in theory we can just show the bound parameters additionally, somewhere, but yea, it for another PR

@richard67
Copy link
Member

in theory we can just show the bound parameters additionally, somewhere, but yea, it for another PR

Yes, that was what I had in mind, too, because parsing the query with a query parser which it not exactly the same parser as running on the server is not safe regarding correctness of the shown information.

I agree it should be another PR.

We just need to be sure we have a release blocker issue for it as long as the other PR is not created, so we don't forget it.

@alikon
Copy link
Contributor

alikon commented Sep 7, 2020

sure me too just want to ask 😄

@alikon
Copy link
Contributor

alikon commented Sep 7, 2020

I have tested this item ✅ successfully on f88bd43


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30580.

@Fedik
Copy link
Member Author

Fedik commented Sep 7, 2020

wait with a test a bit, I need a time to update the rendering for 1 "QUERY PLAN" ;)
I try in next couple days

@alikon
Copy link
Contributor

alikon commented Sep 7, 2020

I have not tested this item.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30580.

@Fedik
Copy link
Member Author

Fedik commented Sep 8, 2020

okay, please test

@richard67
Copy link
Member

I have tested this item ✅ successfully on 8c36884


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30580.

@richard67
Copy link
Member

Hint for other testers: Run composer update joomla/database fur updating the database package after having applied the patch, and then run npm ci or npm run build.js.

@Fedik
Copy link
Member Author

Fedik commented Sep 8, 2020

Hint for other testers: Run composer update joomla/database

it already in PR ;)
just need to run composer install after apply the patch

@richard67
Copy link
Member

@Fedik That works if you have not run it before. But on a 4.0-dev branch where composer install has run before, it needs to just update the database package.

@Fedik
Copy link
Member Author

Fedik commented Sep 8, 2020

okay, I thought it should, well then :)

@alikon
Copy link
Contributor

alikon commented Sep 8, 2020

I have tested this item ✅ successfully on 8c36884


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30580.

@alikon
Copy link
Contributor

alikon commented Sep 8, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30580.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 8, 2020
@laoneo laoneo merged commit 75a4db2 into joomla:4.0-dev Sep 9, 2020
@laoneo
Copy link
Member

laoneo commented Sep 9, 2020

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 9, 2020
@laoneo laoneo added this to the Joomla 4.0 milestone Sep 9, 2020
@Fedik Fedik deleted the debug-explain-fix branch September 9, 2020 09:17
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
* Fix "explain" feature in the debug plugin

* Explain for PostgreSQL

* Update build/media_source/plg_system_debug/widgets/sqlqueries/widget.es5.js

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

* Update rendering of Explain table

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Composer Dependency Changed NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants