? NPM Resource Changed ? ? Pending

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
6 Sep 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

avatar Fedik Fedik - open - 6 Sep 2020
avatar Fedik Fedik - change - 6 Sep 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 6 Sep 2020
Category JavaScript Repository NPM Change External Library Composer Change Front End Plugins
avatar richard67
richard67 - comment - 6 Sep 2020

Setting release blocker label as inherited from the issue.

avatar richard67
richard67 - comment - 6 Sep 2020

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

snap-4

avatar Fedik
Fedik - comment - 6 Sep 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.

avatar richard67
richard67 - comment - 6 Sep 2020

It may take some time until I can do that.

avatar richard67
richard67 - comment - 6 Sep 2020

@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 .

avatar Fedik
Fedik - comment - 6 Sep 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

avatar richard67
richard67 - comment - 6 Sep 2020

@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?

avatar Fedik
Fedik - comment - 6 Sep 2020

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

avatar Fedik Fedik - change - 6 Sep 2020
Labels Added: ? NPM Resource Changed ? ?
avatar Fedik
Fedik - comment - 6 Sep 2020

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

avatar richard67
richard67 - comment - 6 Sep 2020

@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?

avatar Fedik
Fedik - comment - 6 Sep 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)
...
...
avatar Fedik Fedik - change - 6 Sep 2020
Labels Added: ?
Removed: ?
avatar alikon
alikon - comment - 7 Sep 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" 😄

avatar alikon
alikon - comment - 7 Sep 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

avatar richard67
richard67 - comment - 7 Sep 2020

@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?

avatar Fedik
Fedik - comment - 7 Sep 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

avatar richard67
richard67 - comment - 7 Sep 2020

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.

avatar alikon
alikon - comment - 7 Sep 2020

sure me too just want to ask 😄

avatar alikon alikon - test_item - 7 Sep 2020 - Tested successfully
avatar alikon
alikon - comment - 7 Sep 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.

avatar Fedik
Fedik - comment - 7 Sep 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

avatar alikon alikon - test_item - 7 Sep 2020 - Not tested
avatar alikon
alikon - comment - 7 Sep 2020

I have not tested this item.


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

avatar Fedik Fedik - change - 8 Sep 2020
Labels Added: ?
Removed: ?
avatar Fedik
Fedik - comment - 8 Sep 2020

okay, please test

avatar richard67 richard67 - test_item - 8 Sep 2020 - Tested successfully
avatar richard67
richard67 - comment - 8 Sep 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.

avatar richard67
richard67 - comment - 8 Sep 2020

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.

avatar Fedik
Fedik - comment - 8 Sep 2020

Hint for other testers: Run composer update joomla/database

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

avatar richard67
richard67 - comment - 8 Sep 2020

@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.

avatar Fedik
Fedik - comment - 8 Sep 2020

okay, I thought it should, well then :)

avatar alikon alikon - test_item - 8 Sep 2020 - Tested successfully
avatar alikon
alikon - comment - 8 Sep 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.

avatar alikon alikon - change - 8 Sep 2020
Status Pending Ready to Commit
avatar alikon
alikon - comment - 8 Sep 2020

RTC


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

avatar laoneo laoneo - change - 9 Sep 2020
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-09-09 07:55:16
Closed_By laoneo
Labels Added: ?
Removed: ?
avatar laoneo laoneo - close - 9 Sep 2020
avatar laoneo laoneo - merge - 9 Sep 2020
avatar laoneo
laoneo - comment - 9 Sep 2020

Thanks!

Add a Comment

Login with GitHub to post a comment