added feature to create automatic indexing on deleted_at field

[rajentrivedi]

Summary

This merge request introduces an enhancement to Laravel's Blueprint.php by allowing developers to automatically create an index on the deleted_at column whenever they use soft deletes in their database migrations. This change affects the softDeletes(), softDeletesTz(), and softDeletesDatetime() methods.

Changes

The following three methods in Blueprint.php have been updated:

1. softDeletes()
2. softDeletesTz()
3. softDeletesDatetime()

All methods now accept an additional $index parameter (default: true). When set to true, an index will be automatically created on the deleted_at column, improving query performance when querying the records.

The concern is that using the condition WHERE deleted_at IS NULL on large datasets without indexing the deleted_at column leads to performance degradation.

Benefits to Users

1. Improved Query Performance: Adding an index to the deleted_at column improves query performance when filtering by soft-deleted records, especially in large datasets.

2. Developer Convenience: The enhancement simplifies the process for developers, allowing them to add an index with a single parameter rather than manually adding an index in separate migration steps. The $index parameter defaults to true, maintaining the expected behavior. If a user doesn’t want the deleted_at column indexed, they can set $index = false.

Unit Tests

1. PHPUnit tests have been written for all updated methods to ensure proper functionality with and without the index.
2. Tests verify SQL output for both indexed and non-indexed scenarios.

How to use

// Create a soft deletes column with an index (default behavior)
$table->softDeletes();

// Create a soft deletes column without an index
$table->softDeletes('deleted_at', 0, false);

// Similarly, for softDeletesTz and softDeletesDatetime
$table->softDeletesTz('deleted_at', 0, true); // With index
$table->softDeletesDatetime('deleted_at', 0, false); // Without index

[crynobone]

Marking this as draft as test are failing.

[rodrigopedra]

Defaulting the index creation to true is a breaking change, as current applications don't expect that behavior.

Maybe we could default it to false on 11.x, and then change the default to true on 12.x.

[crynobone]

Maybe we could default it to false on 11.x, and then change the default to true on 12.x.

Feel like this is still a bad changes since if you need to create a fresh migration after upgrading to Laravel 12 you will have an inconsistency value between local and production.

[rodrigopedra]

you will have an inconsistency value between local and production.

That is the current issue with adding it defaulting to true, while the current behavior is not to create any index.

Any new developer, installing an existing app, will have a local DB different from production.

On a new version, we can at least document the change.

[rodrigopedra]

A migration like this would break if run on a new installation of a current Laravel 11.x installation:

<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration
{
    public function up(): void
    {
        Schema::create('foo', function (Blueprint $table) {
            $table->id();
            $table->timestamps();
            $table->softDeletes();

            // manually create an index, as previously
            // no index was created by default
            $table->index('deleted_at');
        });
    }

    public function down(): void
    {
        Schema::dropIfExists('foo');
    }
};

$ php artisan migrate

   INFO  Running migrations.  

  2024_09_18_035932_foo ......................................................................................... 21.95ms FAIL

   Illuminate\Database\QueryException 

  SQLSTATE[42000]: Syntax error or access violation: 1061 Duplicate key name 'foo_deleted_at_index' (Connection: mysql, SQL: alter table `foo` add index `foo_deleted_at_index`(`deleted_at`))

  at vendor/laravel/framework/src/Illuminate/Database/Connection.php:825
    821▕                     $this->getName(), $query, $this->prepareBindings($bindings), $e
    822▕                 );
    823▕             }
    824▕ 
  ➜ 825▕             throw new QueryException(
    826▕                 $this->getName(), $query, $this->prepareBindings($bindings), $e
    827▕             );
    828▕         }
    829▕     }

      +9 vendor frames 

  10  database/migrations/2024_09_18_035932_foo.php:11
      Illuminate\Support\Facades\Facade::__callStatic()
      +26 vendor frames 

  37  artisan:13
      Illuminate\Foundation\Application::handleCommand()

On a major version, we can document it on the upgrade guide.

[rodrigopedra]

Feel like this is still a bad changes since if you need to create a fresh migration after upgrading to Laravel 12 you will have an inconsistency value between local and production.

I might have misunderstood your comment, is the idea to change the default to false, and then keep it as false on Laravel 12?

Then one can already do something like this:

$table->softDeletes()->index();

right?

[MehdiAroui]

I guess we don't really need that change, it will make a lot of issues of data consistency between dev end prod, especially if we plan to migrate from a version to another.

[rodrigopedra]

I don't see a problem adding this, we create indices for the ->morphs() columns, and the index's columns switch was done similarly at some time.

I see the ->softDeletes() much like a Blueprint's feature method, such as the ->morphs(), and not as a data type method (e.g. ->string()), where I'd prefer things to be more explicit done.

The only issue I have is changing default behavior on a minor/patch release, instead of in a major release, for such a sensible part of the framework.

[rajentrivedi]

you will have an inconsistency value between local and production.

That is the current issue with adding it defaulting to true, while the current behavior is not to create any index.

Any new developer, installing an existing app, will have a local DB different from production.

On a new version, we can at least document the change.

if we keep default to false, it will not serve the purpose because laravel is not creating index on deleted_at column & this is creating performance issues when application database grows.

when we use SoftDelete, all the queries to the model will be with whereNotNull('deleted_at'), this where clause is slowing down the performance.

[rodrigopedra]

My suggestion is to keep it to false on 11.x, so people are not surprised by changed behavior, as currently no indices are created. Then change it to true on 12.x when people can follow the upgrade guide.

Or maybe send it directly to 12.x instead, as we are approaching the end of the year already, and who wants this right now can use $table->softDeletes()->index() as a workaround.

[rodrigopedra]

Even better, we could send it to master (12.x), always create an index, and allow the developer to customize the index name. Much like how is done with the morphs* methods:

framework/src/Illuminate/Database/Schema/Blueprint.php

Lines 1462 to 1471 in 4c78508

	public function morphs($name, $indexName = null)
	{
	if (Builder::$defaultMorphKeyType === 'uuid') {
	$this->uuidMorphs($name, $indexName);
	} elseif (Builder::$defaultMorphKeyType === 'ulid') {
	$this->ulidMorphs($name, $indexName);
	} else {
	$this->numericMorphs($name, $indexName);
	}
	}

We should allow people to customize the index name, as with long table names the index name can exceed a DBMS naming limit.

Adding the index automatically can then be documented on the upgrade guide, and it will be aligned with the morphs* methods, where the framework is opinionated on the need of an index and how it should be created.

[shaedrich]

Alternatively, there could be a new contract (or attribute) introduced (implements SoftDeletes/#[SoftDeletable]) to be similar to how it is used in models as a concern/trait (use SoftDeletes) 🤔