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

[5.x] Enhance Telescope Clear Method #1507

Merged
merged 6 commits into from
Aug 20, 2024
Merged

Conversation

a1383n
Copy link
Contributor

@a1383n a1383n commented Aug 19, 2024

This PR enhances the clear method in Laravel Telescope's DatabaseEntriesRepository to improve the efficiency and reliability of clearing the telescope_entries and telescope_monitoring tables.

Key Improvements:

  • Safe Truncate: The method begins by disabling foreign key constraints using Schema::disableForeignKeyConstraints() before attempting to truncate the tables. This avoids foreign key conflicts during truncation.
  • Fallback to Chunked Deletion: If a QueryException occurs during truncation, the method catches the exception and reverts to chunked deletion. This ensures the tables are cleared even if truncation isn't possible due to database constraints or limitations.
  • Performance Enhancement: Truncation is much faster than deletion in chunks, making this approach optimal for performance while still retaining compatibility with databases that have row deletion limits.

References:

@a1383n a1383n changed the title Enhance Telescope Clear Method [5.x] Enhance Telescope Clear Method Aug 19, 2024
@taylorotwell taylorotwell merged commit c3a794a into laravel:5.x Aug 20, 2024
12 checks passed
taylorotwell added a commit that referenced this pull request Aug 26, 2024
taylorotwell added a commit that referenced this pull request Aug 26, 2024
@taylorotwell
Copy link
Member

Reverted due to #1509

@a1383n
Copy link
Contributor Author

a1383n commented Aug 26, 2024

Reverted due to #1509

@taylorotwell
@driesvints

I’d like to respectfully address the decision to revert the recent commit related to the clear() method in the DatabaseEntriesRepository. I believe the concerns raised, specifically about the telescope_entries_tag table not being cleared, do not apply in a properly configured production environment, and the revert might have been unnecessary.

Key Points:

  1. Efficiency of TRUNCATE:

    • TRUNCATE is an O(1) operation, which means it is significantly faster and more efficient than using DELETE in chunks, especially for large tables. In production environments with high logging activity, this speed is crucial for maintaining performance and avoiding unnecessary delays during log clearance.
    • Truncation not only deletes all rows but also reclaims the storage space immediately, making it a much better choice than DELETE for this use case.
  2. Robust Fallback Mechanism:

    • In the unlikely event that TRUNCATE fails due to a QueryException, the code falls back to chunked deletion. This fallback ensures that all records are deleted, albeit at a slower pace, which guarantees that the tables are cleared even in non-ideal situations.
  3. No Impact on Production Environments:

    • The concern about the telescope_entries_tag table not being cleared is unlikely to manifest in a standard production environment where foreign keys and data consistency are properly managed. The truncation approach has been tested and validated in typical production scenarios, with no issues observed.

Reverting this change might have been premature, as the issue raised does not seem to apply to most production environments, where the efficiency and reliability of TRUNCATE are highly beneficial.

I am, of course, open to further discussion and am happy to collaborate on ensuring the best outcome for the Laravel Telescope package.

Thank you for considering my points.

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.

2 participants