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

feat: pgsql query plan analyzer support #479

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

p4veI
Copy link
Contributor

@p4veI p4veI commented Nov 20, 2022

Hi, it took me a while to get to this, but I finally managed to take a look at this and I'm keen for some comments regarding the features.

I might have to do a bit more digging regarding postgres indexes, as I currently think the differences are quite big and frankly I'm not sure at this point if this doesn't require a bit different approach - regarding generating tips/generating errors etc.

It's kind of hard for me to figure out if the behaviour I've built in now is somewhat similar to what the mysql analyzer is doing or not, I'll probably discuss with friends in the upcoming week.

So it's kind of a draft for now..

I also noticed pgsql cache is now ignored, what's the reason for that (I guess it complicates maintenance for building mysql features)?

@p4veI p4veI force-pushed the pk-psql-anlyzer branch 2 times, most recently from d0c1853 to ef6edb9 Compare November 20, 2022 20:31
@staabm
Copy link
Owner

staabm commented Nov 20, 2022

Hey!

nice to see you again here :-).

this looks pretty solid to me. I don't have much to add.

I might have to do a bit more digging regarding postgres indexes, as I currently think the differences are quite big and frankly I'm not sure at this point if this doesn't require a bit different approach - regarding generating tips/generating errors etc.

I have 0 experience with pgsql, therefore cannot judge whether this is the way to go. would be great you could double check that with your friends.

I also noticed pgsql cache is now ignored, what's the reason for that (I guess it complicates maintenance for building mysql features)?

I guess you are talking about test coverage for the record and replay functionality?

I don't have pgsql running locally and therefore rely on CI mostly for this stuff. in case we can cover more, without requiring myself to install pgsql locally is very welcome.

thank you

$tip = 'see Mysql Docs https://dev.mysql.com/doc/refman/8.0/en/select-optimization.html';
$tip = 'pdo-pgsql' === getenv('DBA_REFLECTOR')
? QueryPlanResult::PGSQL_TIP
: 'see Mysql Docs https://dev.mysql.com/doc/refman/8.0/en/select-optimization.html';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also put the mysql tip onto QueryPlanResult then

public const NO_INDEX = 'no-index';
public const TABLE_SCAN = 'table-scan';
public const UNINDEXED_READS = 'unindexed-reads';
public const ROW_NO_INDEX = 'no-index';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renaming existing constants is a BC break. Please leave the old ones additionally in and mark them as deprecated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants