Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/Umbraco.Core/Configuration/Models/NuCacheSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ public class NuCacheSettings
/// </summary>
internal const bool StaticUsePagedSqlQuery = true;

/// <summary>
/// The default number of content items whose stale NuCache rows are deleted per batch during a
/// content type rebuild. Matches the SQL parameter limit ceiling the delete is capped to.
/// </summary>
internal const int StaticContentTypeRebuildDeleteBatchSize = 2000;

/// <summary>
/// The serializer type that nucache uses to persist documents in the database.
/// </summary>
Expand All @@ -43,4 +49,20 @@ public class NuCacheSettings
/// </summary>
[DefaultValue(StaticUsePagedSqlQuery)]
public bool UsePagedSqlQuery { get; set; } = true;

/// <summary>
/// Gets or sets the number of content items whose stale NuCache rows are deleted per batch during a
/// content type structural rebuild.
/// </summary>
/// <remarks>
/// Deleting in batches avoids a single unbounded DELETE that can escalate to a table lock, bloat the
/// transaction log, or exceed the command timeout on sites with a lot of content. The matching node ids
/// are read from the source tables once, then their rows are deleted in batches of this size, so the
/// value only bounds the per-statement (and, for a deferred rebuild, per-transaction) footprint — it
/// does not cause repeated scans. The effective size is capped at the SQL parameter limit
/// (<see cref="Constants.Sql.MaxParameterCount" />); lower it if brief lock escalation on
/// <c>cmsContentNu</c> during a rebuild is a concern.
/// </remarks>
[DefaultValue(StaticContentTypeRebuildDeleteBatchSize)]
public int ContentTypeRebuildDeleteBatchSize { get; set; } = StaticContentTypeRebuildDeleteBatchSize;
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Diagnostics;

Check warning on line 1 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (v17/dev)

❌ Getting worse: Code Duplication

introduced similar code in: CountByObjectType,GetNodeIdsByContentTypes,GetPagedContentNodeIds Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using NPoco;
Expand Down Expand Up @@ -444,45 +444,27 @@
}

Guid contentObjectType = Constants.ObjectTypes.Document;

long total = 0;
Dictionary<int, byte>? contentTypeVariations = null;
Dictionary<short, string>? languageMap = null;
Dictionary<int, List<PropertyTypeInfo>>? propertyInfoByContentType = null;

// Delete and pre-fetch in one step, then release locks before the paging loop.
executeStep(() =>
{
RemoveByObjectType(contentObjectType, contentTypeIds);

Sql<ISqlContext> countSql = Sql()
.SelectCount()
.From<NodeDto>()
.InnerJoin<ContentDto>().On<NodeDto, ContentDto>((n, c) => n.NodeId == c.NodeId)
.Where<NodeDto>(x => x.NodeObjectType == contentObjectType);

if (contentTypeIds.Count > 0)
{
countSql = countSql.WhereIn<ContentDto>(x => x.ContentTypeId, contentTypeIds);
}
// Delete stale rows in batches (each its own step); the returned count of affected nodes is the
// number to repopulate, so no separate count query is needed.
long total = RemoveByObjectTypeInBatches(contentObjectType, contentTypeIds, executeStep);

total = Database.ExecuteScalar<long>(countSql);

if (total == 0)
{
return;
}
if (total == 0)
{
return;
}

executeStep(() =>
{
contentTypeVariations = GetContentTypeVariations(contentTypeIds);
languageMap = GetLanguageMap();
propertyInfoByContentType = GetPropertyInfoByContentType(contentTypeIds);
});

Check notice on line 467 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (v17/dev)

✅ No longer an issue: Large Method

RebuildContentDbCache is no longer above the threshold for lines of code Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
if (total == 0)
{
return;
}

long processed = 0;
long pageIndex = 0;

Expand Down Expand Up @@ -526,7 +508,7 @@
pageComplete = true;
});

if (!pageComplete)

Check warning on line 511 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable.

See more on https://sonarcloud.io/project/issues?id=umbraco_Umbraco-CMS&issues=AZ9B-agmw_HdArncaEFn&open=AZ9B-agmw_HdArncaEFn&pullRequest=23329
{
break;
}
Expand Down Expand Up @@ -1023,39 +1005,22 @@

Guid mediaObjectType = Constants.ObjectTypes.Media;

long total = 0;
Dictionary<int, List<string>>? propertyAliasesByContentType = null;

executeStep(() =>
{
RemoveByObjectType(mediaObjectType, contentTypeIds);

Sql<ISqlContext> countSql = Sql()
.SelectCount()
.From<NodeDto>()
.InnerJoin<ContentDto>().On<NodeDto, ContentDto>((n, c) => n.NodeId == c.NodeId)
.Where<NodeDto>(x => x.NodeObjectType == mediaObjectType);

if (contentTypeIds.Count > 0)
{
countSql = countSql.WhereIn<ContentDto>(x => x.ContentTypeId, contentTypeIds);
}

total = Database.ExecuteScalar<long>(countSql);

if (total == 0)
{
return;
}

propertyAliasesByContentType = GetPropertyAliasesByContentType(contentTypeIds);
});
// Delete stale rows in batches (each its own step); the returned count of affected nodes is the
// number to repopulate, so no separate count query is needed.
long total = RemoveByObjectTypeInBatches(mediaObjectType, contentTypeIds, executeStep);

if (total == 0)
{
return;
}

executeStep(() =>
{
propertyAliasesByContentType = GetPropertyAliasesByContentType(contentTypeIds);
});

long processed = 0;
long pageIndex = 0;

Expand Down Expand Up @@ -1086,7 +1051,7 @@
pageComplete = true;
});

if (!pageComplete)

Check warning on line 1054 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable.

See more on https://sonarcloud.io/project/issues?id=umbraco_Umbraco-CMS&issues=AZ9B-agmw_HdArncaEFo&open=AZ9B-agmw_HdArncaEFo&pullRequest=23329
{
break;
}
Expand Down Expand Up @@ -1344,38 +1309,22 @@

Guid memberObjectType = Constants.ObjectTypes.Member;

long total = 0;
Dictionary<int, List<string>>? propertyAliasesByContentType = null;

executeStep(() =>
{
RemoveByObjectType(memberObjectType, contentTypeIds);

Sql<ISqlContext> countSql = Sql()
.SelectCount()
.From<NodeDto>()
.InnerJoin<ContentDto>().On<NodeDto, ContentDto>((n, c) => n.NodeId == c.NodeId)
.Where<NodeDto>(x => x.NodeObjectType == memberObjectType);

if (contentTypeIds.Count > 0)
{
countSql = countSql.WhereIn<ContentDto>(x => x.ContentTypeId, contentTypeIds);
}

total = Database.ExecuteScalar<long>(countSql);
if (total == 0)
{
return;
}

propertyAliasesByContentType = GetPropertyAliasesByContentType(contentTypeIds);
});
// Delete stale rows in batches (each its own step); the returned count of affected nodes is the
// number to repopulate, so no separate count query is needed.
long total = RemoveByObjectTypeInBatches(memberObjectType, contentTypeIds, executeStep);

if (total == 0)
{
return;
}

executeStep(() =>
{
propertyAliasesByContentType = GetPropertyAliasesByContentType(contentTypeIds);
});

long processed = 0;
long pageIndex = 0;

Expand Down Expand Up @@ -1406,7 +1355,7 @@
pageComplete = true;
});

if (!pageComplete)

Check warning on line 1358 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable.

See more on https://sonarcloud.io/project/issues?id=umbraco_Umbraco-CMS&issues=AZ9B-agmw_HdArncaEFp&open=AZ9B-agmw_HdArncaEFp&pullRequest=23329
{
break;
}
Expand All @@ -1415,6 +1364,112 @@
}
}

/// <summary>
/// Deletes the <c>cmsContentNu</c> rows for the given object type in batches, running each batch through
/// the supplied <paramref name="executeStep" /> delegate.
/// </summary>
/// <remarks>
/// Batching avoids a single unbounded DELETE that can escalate to a table lock, bloat the transaction
/// log, and (for a content type backing a lot of content) exceed the command timeout. When the caller's
/// <paramref name="executeStep" /> opens a fresh scope per step (the deferred rebuild path), each batch
/// commits in its own transaction, so the locks and log space it uses are released between batches
/// instead of accumulating across the whole delete. When it runs inline (the immediate path) the batches
/// share the ambient transaction, but each DELETE statement is still individually bounded.
/// </remarks>
/// <returns>The number of content nodes affected, i.e. the number that will need repopulating.</returns>
private long RemoveByObjectTypeInBatches(Guid objectType, IReadOnlyCollection<int> contentTypeIds, Action<Action> executeStep)
{
// A full clear (no content type filter) is only used by full rebuilds, where the table has typically
// already been truncated, so keep it as a single statement.
if (contentTypeIds.Count == 0)
{
long allCount = 0;
executeStep(() =>
{
RemoveByObjectType(objectType, contentTypeIds);
allCount = CountByObjectType(objectType, contentTypeIds);
});
return allCount;
}

// The node ids come from the (stable) source tables, not cmsContentNu, so fetching them once up front
// is safe: concurrent foreground saves during a deferred rebuild only add/remove rows we either
// correctly skip (new rows are preserved) or harmlessly no-op on (already-removed rows).
List<int> nodeIds = [];
executeStep(() => nodeIds = GetNodeIdsByContentTypes(objectType, contentTypeIds));

var batchSize = Math.Clamp(_nucacheSettings.Value.ContentTypeRebuildDeleteBatchSize, 1, Constants.Sql.MaxParameterCount);
var totalBatches = (int)Math.Ceiling(nodeIds.Count / (double)batchSize);

_logger.LogDebug(
"Rebuild: deleting cmsContentNu rows for object type {ObjectType} — {NodeCount} node(s) in {BatchCount} batch(es) of up to {BatchSize}.",
objectType,
nodeIds.Count,
totalBatches,
batchSize);

Check warning on line 1409 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Evaluation of this argument may be expensive and unnecessary if logging is disabled

See more on https://sonarcloud.io/project/issues?id=umbraco_Umbraco-CMS&issues=AZ9B-agmw_HdArncaEFq&open=AZ9B-agmw_HdArncaEFq&pullRequest=23329

var batchNumber = 0;
foreach (IEnumerable<int> batch in nodeIds.InGroupsOf(batchSize))
{
var nodeIdBatch = batch.ToArray();
var currentBatchNumber = ++batchNumber;
executeStep(() =>
{
DeleteContentNuByNodeIds(nodeIdBatch);
_logger.LogDebug(
"Rebuild: deleted cmsContentNu batch {BatchNumber}/{BatchCount} ({NodeCount} node(s)) for object type {ObjectType}.",
currentBatchNumber,
totalBatches,
nodeIdBatch.Length,
objectType);

Check warning on line 1424 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Evaluation of this argument may be expensive and unnecessary if logging is disabled

See more on https://sonarcloud.io/project/issues?id=umbraco_Umbraco-CMS&issues=AZ9B-agmw_HdArncaEFr&open=AZ9B-agmw_HdArncaEFr&pullRequest=23329
});
}

return nodeIds.Count;
}

private List<int> GetNodeIdsByContentTypes(Guid objectType, IReadOnlyCollection<int> contentTypeIds)
{
Sql<ISqlContext> sql = Sql()
.Select<NodeDto>(x => x.NodeId)
.From<NodeDto>()
.InnerJoin<ContentDto>().On<NodeDto, ContentDto>((n, c) => n.NodeId == c.NodeId)
.Where<NodeDto>(x => x.NodeObjectType == objectType)
.WhereIn<ContentDto>(x => x.ContentTypeId, contentTypeIds);

return Database.Fetch<int>(sql);
}

private long CountByObjectType(Guid objectType, IReadOnlyCollection<int> contentTypeIds)
{
Sql<ISqlContext> sql = Sql()
.SelectCount()
.From<NodeDto>()
.InnerJoin<ContentDto>().On<NodeDto, ContentDto>((n, c) => n.NodeId == c.NodeId)
.Where<NodeDto>(x => x.NodeObjectType == objectType);

if (contentTypeIds.Count > 0)
{
sql = sql.WhereIn<ContentDto>(x => x.ContentTypeId, contentTypeIds);
}

return Database.ExecuteScalar<long>(sql);
}

private void DeleteContentNuByNodeIds(IReadOnlyCollection<int> nodeIds)

Check warning on line 1459 in src/Umbraco.PublishedCache.HybridCache/Persistence/DatabaseCacheRepository.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Change type of parameter 'nodeIds' from 'System.Collections.Generic.IReadOnlyCollection<int>' to 'int[]' for improved performance

See more on https://sonarcloud.io/project/issues?id=umbraco_Umbraco-CMS&issues=AZ9B-agmw_HdArncaEFs&open=AZ9B-agmw_HdArncaEFs&pullRequest=23329
{
if (nodeIds.Count == 0)
{
return;
}

Sql<ISqlContext> sql = Sql()
.Delete<ContentNuDto>()
.WhereIn<ContentNuDto>(x => x.NodeId, nodeIds);

Database.Execute(sql);
}

private void RemoveByObjectType(Guid objectType, IReadOnlyCollection<int> contentTypeIds)
{
// If the provided contentTypeIds collection is empty, remove all records for the provided object type.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
using Microsoft.Extensions.DependencyInjection;
using NUnit.Framework;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Cache;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Notifications;
using Umbraco.Cms.Core.PublishedCache;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Sync;
using Umbraco.Cms.Infrastructure.HybridCache.Persistence;
using Umbraco.Cms.Infrastructure.HybridCache.Serialization;
using Umbraco.Cms.Infrastructure.Persistence;
using Umbraco.Cms.Infrastructure.Persistence.Dtos;
using Umbraco.Cms.Tests.Common.Builders;
using Umbraco.Cms.Tests.Common.Builders.Extensions;
using Umbraco.Cms.Tests.Common.Testing;
Expand All @@ -25,10 +29,17 @@ protected override void CustomTestSetup(IUmbracoBuilder builder)
builder.AddNotificationHandler<ContentTreeChangeNotification, ContentTreeChangeDistributedCacheNotificationHandler>();
builder.AddNotificationHandler<MediaTreeChangeNotification, MediaTreeChangeDistributedCacheNotificationHandler>();
builder.Services.AddUnique<IServerMessenger, ContentEventsTests.LocalServerMessenger>();

// Force several delete batches over the handful of fixture documents so the batched delete loop is exercised.
builder.Services.PostConfigure<NuCacheSettings>(options => options.ContentTypeRebuildDeleteBatchSize = 2);
}

private IDatabaseCacheRepository DatabaseCacheRepository => GetRequiredService<IDatabaseCacheRepository>();

private IDocumentCacheService DocumentCacheService => GetRequiredService<IDocumentCacheService>();

private ISqlContext SqlContext => GetRequiredService<ISqlContext>();

private IContentPublishingService ContentPublishingService => GetRequiredService<IContentPublishingService>();

private IMediaTypeService MediaTypeService => GetRequiredService<IMediaTypeService>();
Expand Down Expand Up @@ -107,6 +118,54 @@ public async Task GetMediaSourcesAsync_With_Empty_Keys_Returns_Nothing()
Assert.That(sources, Is.Empty);
}

[Test]
public void Rebuild_Deletes_Stale_Rows_In_Batches_And_Repopulates()
Comment thread
AndyButland marked this conversation as resolved.
{
// Arrange — populate the cache for the document type, then mark every row for the type as stale.
// If the batched delete failed to remove a row, the repopulation's insert-where-not-exists would
// skip it and the stale marker would survive — so this proves the delete actually runs (with a
// batch size of 2 forcing multiple batches over the fixture's documents).
DocumentCacheService.Rebuild([ContentType.Id]);

const string staleMarker = "STALE";
var contentTypeNodeIds = new[] { Textpage.Id, Subpage.Id, Subpage2.Id, Subpage3.Id };

using (var scope = ScopeProvider.CreateScope())
{
ScopeAccessor.AmbientScope!.Database.Execute(
SqlContext.Sql()
.Update<ContentNuDto>(u => u.Set(x => x.Data, staleMarker))
.WhereIn<ContentNuDto>(x => x.NodeId, contentTypeNodeIds));
scope.Complete();
}

// Act
DocumentCacheService.Rebuild([ContentType.Id]);

// Assert — every document of the type has a refreshed (non-stale) row, and none was left behind.
using (var scope = ScopeProvider.CreateScope(autoComplete: true))
{
var dtos = ScopeAccessor.AmbientScope!.Database.Fetch<ContentNuDto>(
SqlContext.Sql()
.Select<ContentNuDto>()
.From<ContentNuDto>()
.WhereIn<ContentNuDto>(x => x.NodeId, contentTypeNodeIds));

Assert.Multiple(() =>
{
Assert.That(dtos, Is.Not.Empty, "Expected the cache to be repopulated for the content type.");
Assert.That(
dtos.Select(x => x.NodeId).Distinct(),
Is.EquivalentTo(contentTypeNodeIds),
"Every document of the type should have a cache row after the rebuild.");
Assert.That(
dtos.Any(x => x.Data == staleMarker),
Is.False,
"The batched delete should have removed the stale rows before repopulation.");
});
}
}

[Test]
public void GetContentByContentTypeKey_With_Keys_Returns_Matching_Nodes()
{
Expand Down
Loading