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.

Sign up for GitHub

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

Jump to bottom

Add primary key on temp table #9770

Merged
merged 2 commits into from
Jun 2, 2022
Merged

Add primary key on temp table #9770

merged 2 commits into from
Jun 2, 2022

Conversation

wiseguy1394
Copy link
Contributor

@wiseguy1394 wiseguy1394 commented May 20, 2022 •
edited by derrabus
Loading

… to prevent exception on sql_require_primary_key=ON

Fixes #9768

@derrabus derrabus added the Bug label May 20, 2022
@derrabus derrabus changed the title Issue 9768: add primary key on temp table to prevent exception on sql_require_primary_key=ON Add primary key on temp table May 20, 2022
@derrabus
Copy link
Member

derrabus commented May 20, 2022 •
edited
Loading

This PR adds the PK no matter what database engine we're using, right? Given that we use that temp table in an IN() statement later on, it might be a good idea to have the PK anyway, even if the sql_require_primary_key restriction is not in place.

Opinions on this, @greg0ire @beberlei?

I think we should enable sql_require_primary_key for MySQL 8 in our CI to make sure we don't introduce further regressions with that setting.

@wiseguy1394 Can you please run ./vendor/bin/phpcbf and commit the result?

@greg0ire
Copy link
Member

I don't have much to say on this, it seems fine, and I can't see how it would hurt.

@greg0ire greg0ire requested a review from derrabus May 21, 2022 13:32
@greg0ire greg0ire enabled auto-merge May 21, 2022 13:32
@greg0ire
Copy link
Member

Please associate your email address with your Github account, or change the
email in your commits to an address already associated with it. If you do not
want to expose your personal email address, you may use
[email protected], that way we can still reach you through Github.
Additionally, you will get credit for this contribution on your Github profile.

@greg0ire greg0ire disabled auto-merge May 21, 2022 13:33
@wiseguy1394
Copy link
Contributor Author

Ah ok.. sry..I tried to changed the author for my commit, hope it worked.

@greg0ire
Copy link
Member

greg0ire commented May 21, 2022 •
edited
Loading

It resulted in 3 commits, because I think you reset the author, then tried to push and noticed my commit, and you merged them both. I put it back to 1 commit.

Please try again, but after doing: git fetch --all && git reset --hard e06e215. This will effectively use the commit I just pushed as a starting point.

beberlei
beberlei requested changes May 22, 2022
View reviewed changes
Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

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

Wouldn't this need all columns of the primary key of the entity instead just the first one? Otherwise it will lead to primary key duplicates for entities with multiple columns.

Does the MultiTableDeleteExecutor require the same change?

This will probably increase performance of these commands.

@greg0ire greg0ire requested a review from beberlei May 23, 2022 07:28
@wiseguy1394
Copy link
Contributor Author

Is there anything I must/could do?

@derrabus
Copy link
Member

derrabus commented Jun 2, 2022

Is there anything I must/could do?

You could answer @beberlei's questions. 😉

@wiseguy1394
Copy link
Contributor Author

Is there anything I must/could do?

You could answer @beberlei's questions. 😉

Oh ok, I answered the questions with yes in my commits ;)

beberlei
beberlei approved these changes Jun 2, 2022
View reviewed changes
Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

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

Thank you!

@derrabus derrabus merged commit 768e2f3 into doctrine:2.12.x Jun 2, 2022
@wiseguy1394 wiseguy1394 deleted the issue-9768 branch June 2, 2022 14:16
derrabus added a commit to derrabus/orm that referenced this pull request Jun 3, 2022
@derrabus
Merge branch '2.13.x' into 3.0.x
b8299fe 
* 2.13.x:
  PHPStan 1.7.9 (doctrine#9812)
  Remove empty test file (doctrine#9805)
  Remove calls to deprecated MockBuilder::setMethods() (doctrine#9808)
  Deprecate passing null to Query::setFirstResult()
  Rename Abstract*Test to *TestCase (doctrine#9806)
  Add primary key on temp table (doctrine#9770)
  Fix wrong types (doctrine#9802)
  Widen return type
  Update baseline
  Fix
  Add type for AssociationMapping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus approved these changes

@beberlei beberlei beberlei approved these changes

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sql_require_primary_key on mysql 8
4 participants
@wiseguy1394 @derrabus @greg0ire @beberlei