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

Adding an index for the address_log_entries #16096

Open
wants to merge 2 commits into
base: 2.0
Choose a base branch
from

Conversation

mamazu
Copy link
Member

@mamazu mamazu commented Apr 5, 2024

Q A
Branch? 1.13
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets -
License MIT

What's inside

Adding indicies to the address_order_log_entry.

Why?

I don't know how exactly gedomo's loggable trait works but appereantly it querries the current version for an object like this:

SELECT MAX(s?_.version) AS sclr_? 
FROM sylius_address_log_entries s?_ 
WHERE s?_.object_id = ? AND s?_.object_class = ?

Which is very expensive if you have a lot of entries.

@mamazu mamazu requested review from a team as code owners April 5, 2024 12:11
@mamazu mamazu force-pushed the indexing_address_log_entries branch from 5a56767 to 581201f Compare April 5, 2024 12:15
@mamazu mamazu force-pushed the indexing_address_log_entries branch from 581201f to 62b2f02 Compare April 5, 2024 12:18
Copy link

github-actions bot commented Apr 5, 2024

Bunnyshell Preview Environment deployed

It will be automatically stopped in 4 hours.

Use the command /bns:start to start it if it's stopped.

Component Endpoints
mailhog https://mailhog-cvhist.bunnyenv.com/
php https://store-cvhist.bunnyenv.com/

Available commands:

  • /bns:stop to stop the environment
  • /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

NoResponseMate
NoResponseMate previously approved these changes Apr 5, 2024
Copy link
Contributor

@Rafikooo Rafikooo left a comment

Choose a reason for hiding this comment

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

Overall, this PR is ok, but it should be targeted for the 2.0 branch.

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

final class Version20240405121345 extends AbstractMigration
Copy link
Contributor

Choose a reason for hiding this comment

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

Starting from Sylius 1.13, we need to create two migration versions, one for MySQL and one for PostgreSQL. Here's an example for PostgreSQL:

final class Version20240318083808 extends AbstractPostgreSQLMigration

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a way to generate it with doctrine?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seeing that the syntax for creating an index on prostgres is the same. I just extended the postgres migration.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Rafikooo I've changed the base of the PR to 2.0 and use an AbstractPostgreSQLMigration.

@mamazu mamazu changed the base branch from 1.13 to 2.0 May 15, 2024 12:43
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

3 participants