Add warning about varchar(max)

[mkincaid-bw]

📔 Objective

Added clarification when adding new columns to a table using varchar(max)/nvarchar(max) vs varchar(n)/nvarchar(n), especially in regards to NOT NULL columns with default values.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
  team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
  issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @mkincaid-bw's task —— View job

---

PR Review: Add warning about varchar(max)

Todo List

• Read repository guidelines from CLAUDE.md
• Examine the specific changes made to sql.md
• Review content for accuracy, clarity, and formatting
• Check adherence to documentation standards
• Provide comprehensive feedback

Review Feedback

This PR adds a helpful warning section about VARCHAR(MAX)/NVARCHAR(MAX) usage in SQL documentation. Overall, the addition is valuable and addresses an important performance consideration. Here's my detailed review:

✅ Strengths

1. Important technical content: The warning addresses a real performance issue with VARCHAR(MAX)/NVARCHAR(MAX) columns combined with NOT NULL defaults.

2. Good placement: The warning is well-positioned in the "Adding a column" section at docs/contributing/code-style/sql.md:485-497 where developers would encounter it when making schema changes.

3. Proper formatting: Uses Docusaurus :::warning admonition syntax correctly, which aligns with the repository's documentation patterns.

4. Clear structure: The content is well-organized with explanation followed by recommended alternatives.

⚠️ Issues Requiring Attention

1. Technical inaccuracy in line 493:

   Instead, use appropriately-sized `VARCHAR(MAX)`/`NVARCHAR(MAX)` columns based on the expected data length.
   

   This sentence is confusing and technically incorrect. It should read:

   Instead, use appropriately-sized `VARCHAR(n)`/`NVARCHAR(n)` columns based on the expected data length.
   

   The recommendation should be to use fixed-size varchar columns (e.g., VARCHAR(255)) rather than VARCHAR(MAX).

2. Unclear explanation of the problem: The technical explanation could be clearer about why this combination is problematic. The current text about "LOB pointers" and "metadata reserves space" might be confusing to developers not familiar with SQL Server internals.

🎨 Suggestions for Improvement

1. Clarify the recommendation (line 493): Replace the confusing sentence with something like:

   Instead, use fixed-size `VARCHAR(n)`/`NVARCHAR(n)` columns (e.g., `VARCHAR(255)`) based on the expected maximum data length.
   

2. Simplify the technical explanation: Consider making the explanation more accessible:

   `VARCHAR(MAX)`/`NVARCHAR(MAX)` columns can cause significant performance overhead, especially when combined with `NOT NULL` and a default value. SQL Server treats these as large object (LOB) columns, which require additional storage overhead and can impact query performance, even when storing small values.
   

3. Consider adding a positive example: Show what developers should do instead:

   -- ❌ Avoid this
   ALTER TABLE [User] ADD [Notes] NVARCHAR(MAX) NOT NULL DEFAULT '';
   
   -- ✅ Prefer this
   ALTER TABLE [User] ADD [Notes] NVARCHAR(500) NOT NULL DEFAULT '';

📝 Minor Notes

• The content follows the repository's style guide well (concise paragraphs, proper heading capitalization)
• No spelling or grammar issues detected
• The warning placement and formatting align with established documentation patterns

Recommendation

The PR adds valuable content but requires one critical fix to the technical accuracy in line 493. Once that's corrected, this will be a solid addition to the documentation that helps prevent a common performance anti-pattern.

---

[cloudflare-workers-and-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	0f3ffd2
Status:	✅  Deploy successful!
Preview URL:	https://ac5ce9de.contributing-docs.pages.dev
Branch Preview URL:	https://sql-varchar-max.contributing-docs.pages.dev

View logs

[claude]

Claude finished @mkincaid-bw's task —— View job

---

PR Review: Add warning about varchar(max)

Todo List

• Read repository guidelines from CLAUDE.md
• Examine the specific changes made to sql.md
• Review content for accuracy, clarity, and formatting
• Check adherence to documentation standards
• Provide comprehensive feedback

I'm analyzing the changes to provide a thorough review of the varchar(max) documentation addition.

[github-actions]

Checkmarx One – Scan Summary & Details – 72c8b340-9195-416f-89df-b97b26f7c1dd

Great job! No new security vulnerabilities introduced in this pull request

[claude]

Claude finished @mkincaid-bw's task —— View job

---

PR Review: Add warning about varchar(max)

Todo List

• Read repository guidelines from CLAUDE.md
• Examine the specific changes made to sql.md
• Review content for accuracy, clarity, and formatting
• Check adherence to documentation standards
• Provide comprehensive feedback

Review Feedback

This PR adds a helpful warning section about VARCHAR(MAX)/NVARCHAR(MAX) usage in SQL documentation. Overall, the addition is valuable and addresses an important performance consideration. Here's my detailed review:

✅ Strengths

1. Important technical content: The warning addresses a real performance issue with VARCHAR(MAX)/NVARCHAR(MAX) columns combined with NOT NULL defaults.

2. Good placement: The warning is well-positioned in the "Adding a column" section at docs/contributing/code-style/sql.md:485-504 where developers would encounter it when making schema changes.

3. Proper formatting: Uses Docusaurus :::warning admonition syntax correctly, which aligns with the repository's documentation patterns.

4. Clear structure: The content is well-organized with explanation followed by practical examples showing both what to avoid and what to prefer.

5. Follows repository style: Adheres to CLAUDE.md guidelines:

   • Uses code blocks for SQL examples (not inline code)
   • Keeps paragraphs concise and to-the-point
   • Uses proper heading capitalization
   • Provides actionable guidance

✅ Technical Accuracy

The technical content is accurate:

• Correctly identifies the LOB storage overhead issue
• Properly explains the performance implications
• Provides sound alternatives with fixed-size VARCHAR(n)/NVARCHAR(n)
• The code examples are syntactically correct and follow established patterns

✅ Content Quality

1. Clear explanation: The warning effectively explains why this combination is problematic without getting too deep into SQL Server internals.

2. Practical examples: The before/after code examples clearly demonstrate the issue and solution:

   -- Avoid this
   ALTER TABLE [User] ADD [Notes] NVARCHAR(MAX) NOT NULL DEFAULT '';
   
   -- Prefer this
   ALTER TABLE [User] ADD [Notes] NVARCHAR(500) NOT NULL DEFAULT '';

3. Balanced guidance: Acknowledges legitimate use cases for unlimited length while warning about the performance trade-offs.

📝 Minor Observations

1. Context integration: The warning fits naturally within the existing "Adding a column" section's discussion of NOT NULL columns and default constraints.

2. Consistent terminology: Uses consistent SQL Server terminology (VARCHAR(MAX), NVARCHAR(MAX), LOB columns).

3. Appropriate scope: Focuses specifically on the problematic combination rather than blanket advice against VARCHAR(MAX).

Overall Assessment

This is a high-quality addition that addresses a common performance anti-pattern in SQL Server. The content is technically accurate, well-formatted, follows repository conventions, and provides clear, actionable guidance. The warning is appropriately placed where developers making schema changes would see it.

Recommendation: ✅ Approve - This change improves the documentation by helping developers avoid a subtle but significant performance issue.

---

[withinfocus]

Can you share where not null max defaults are in use? I would honestly say there should never be a case where we do this and any calling code should be able to handle null values.

[mkincaid-bw]

Can you share where not null max defaults are in use? I would honestly say there should never be a case where we do this and any calling code should be able to handle null values.

None of the current NOT NULL varchar(max)/nvarchar(max) columns have default constraints, but I was recently asked about adding a new nvarchar column with a default value to the User table. We have a fair amount of varchar(max)/nvarchar(max) (both NULL and NOT NULL) in the database, so I wanted to point out that when adding a new varchar/nvarchar column to a table, ideally it should be varchar(n)/nvarchar(n) vs varchar(max)/nvarchar(max), and the Contributing Docs didn't really address this.

[withinfocus]

I'd suggest changing the language to state that defaults are not to be provided for strings then, plain and simple, and regardless of max being used. My personal opinion -- and I suppose this could be specified here too -- is that only integral types can utilize defaults, so practically speaking it's just bits and ints.

[claude]

Claude finished @mkincaid-bw's task —— View job

---

PR Review: Add warning about varchar(max)

Todo List

• Read repository guidelines from CLAUDE.md
• Examine the PR changes to sql.md
• Review content for technical accuracy and clarity
• Check adherence to documentation standards
• Provide comprehensive feedback considering previous discussions

Review Feedback

This PR has evolved significantly from its original varchar(max) focus to address a broader policy about string column defaults. Based on the discussion thread and the current implementation, here's my comprehensive review:

✅ Strengths

1. Addresses the root issue: The warning now tackles the fundamental problem raised by @withinfocus - that defaults should not be used for string columns at all, regardless of whether they're VARCHAR(MAX) or fixed-size.

2. Clear and concise policy: The warning is well-positioned in the "Adding a column" section at docs/contributing/code-style/sql.md:481-487 and provides clear guidance.

3. Proper formatting: Uses Docusaurus :::warning admonition syntax correctly, following repository conventions.

4. Follows CLAUDE.md guidelines:

   • Uses code blocks appropriately
   • Keeps content concise and to-the-point
   • Proper heading capitalization
   • Clear, actionable guidance

🎨 Technical Assessment

The current warning states:

Default values should only be used for integral types (BIT, TINYINT, SMALLINT, INT, BIGINT). 
Do not provide default values for string columns (VARCHAR, NVARCHAR, or their MAX variants), 
as this can lead to unnecessary storage overhead and performance issues.

Technical accuracy: ✅ Sound approach

• Restricting defaults to integral types is a conservative, performance-oriented policy
• Eliminates the varchar(max) performance issues entirely by avoiding defaults on all string types
• Prevents subtle storage overhead issues across all string column types

📝 Alignment with Discussion Context

The current implementation directly addresses the feedback from the PR discussion:

1. @withinfocus suggestion: "I'd suggest changing the language to state that defaults are not to be provided for strings then, plain and simple" - ✅ Implemented

2. @mkincaid-bw's original concern: About varchar(max) with defaults - ✅ Addressed by preventing the problematic pattern entirely

3. Broader applicability: The policy now covers all string types, not just varchar(max), making it more comprehensive

⚠️ Considerations

1. Policy scope: This is a significant policy change that restricts defaults for all string columns. While technically sound, it's worth confirming this aligns with Bitwarden's broader database practices.

2. Migration impact: Existing code that uses string defaults would need to be refactored over time, though the policy prevents new occurrences.

3. Alternative approaches: Applications will need to handle null string values or use application-level defaults instead of database defaults.

🔍 Code Quality Review

• Syntax: Warning syntax is correct for Docusaurus
• Placement: Appropriately positioned in the column addition section
• Clarity: Message is clear and unambiguous
• Style: Follows established documentation patterns

Overall Assessment

This is a well-executed evolution of the original PR that addresses the core performance concerns while implementing a cleaner, more comprehensive policy. The change from the specific varchar(max) warning to a general "no string defaults" policy is technically sound and addresses the root cause rather than just one symptom.

Recommendation: ✅ Approve

The current implementation is technically accurate, follows documentation standards, and directly addresses the concerns raised in the discussion. It provides clear, actionable guidance that will prevent both the original varchar(max) performance issues and similar problems with other string types.