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

Add batched migration script #135

Closed

Conversation

Arkatufus
Copy link
Contributor

Changes

Add batched migration script

@Arkatufus Arkatufus requested a review from to11mtm February 3, 2023 19:04
Copy link
Member

@to11mtm to11mtm left a comment

Choose a reason for hiding this comment

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

@Arkatufus I'm looking at these with the mindset that they are expected to be modified for a specific use case.

That aside,

  • We should give our function names unique names if we are using CREATE OR ALTER, especially in the case of functions named things like Split which may have an existing declaration in a Prod DB.
  • We should have documentation that makes it abundantly clear to users that these scripts must be modified for their specific table configuration(s).

BEGIN
BEGIN TRAN;
INSERT INTO [dbo].[tags]([ordering_id], [tag])
SELECT * FROM (SELECT records.[Ordering], cross_product.[items] FROM (
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer specific columns over select * here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SELECT * is actually nescessary here, the specific columns are defined inside the inner SELECT

WHERE ej.[Ordering] >= @from_id AND ej.[Ordering] <= @from_id + 1000
) AS records CROSS APPLY [dbo].[Split](records.Tags, ';') cross_product) AS s([ordering_id], [tag])
WHERE NOT EXISTS (
SELECT * FROM [dbo].[tags] t WITH (updlock)
Copy link
Member

Choose a reason for hiding this comment

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

You can probably SELECT 1 instead of SELECT * here.

@Arkatufus
Copy link
Contributor Author

@to11mtm makes sense, I'll try to incorporate those changes

@Aaronontheweb
Copy link
Member

@Arkatufus needs a rebase

@Arkatufus
Copy link
Contributor Author

Arkatufus commented Mar 21, 2023

Rebased to recent dev branch in #168

@Arkatufus Arkatufus closed this Mar 21, 2023
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.

3 participants