perf: Drop unused/duplicate/sub-optimal indexes (backport #38884) (#38913)

perf: Drop unused/duplicate/sub-optimal indexes (#38884)

* ci: enable more checks

* perf: Drop unused/duplicate indexes

(cherry picked from commit 787333896c)

Co-authored-by: Ankush Menat <ankush@frappe.io>

.pre-commit-config.yaml

@@ -5,7 +5,7 @@ fail_fast: false
 
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v4.0.1
+    rev: v4.3.0
     hooks:
       - id: trailing-whitespace
         files: "erpnext.*"
@@ -15,6 +15,10 @@ repos:
         args: ['--branch', 'develop']
       - id: check-merge-conflict
       - id: check-ast
+      - id: check-json
+      - id: check-toml
+      - id: check-yaml
+      - id: debug-statements
 
   - repo: https://github.com/pre-commit/mirrors-eslint
     rev: v8.44.0

erpnext/accounts/doctype/gl_entry/gl_entry.json

@@ -142,8 +142,7 @@
    "label": "Against Voucher Type",
    "oldfieldname": "against_voucher_type",
    "oldfieldtype": "Data",
-   "options": "DocType",
+   "options": "DocType"
-   "search_index": 1
   },
   {
    "fieldname": "against_voucher",
@@ -162,8 +161,7 @@
    "label": "Voucher Type",
    "oldfieldname": "voucher_type",
    "oldfieldtype": "Select",
-   "options": "DocType",
+   "options": "DocType"
-   "search_index": 1
   },
   {
    "fieldname": "voucher_no",
@@ -321,4 +319,4 @@
  "sort_field": "modified",
  "sort_order": "DESC",
  "states": []
-}
+}

erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py

@@ -1816,10 +1816,6 @@ def make_inter_company_sales_invoice(source_name, target_doc=None):
 	return make_inter_company_transaction("Purchase Invoice", source_name, target_doc)
 
 
-def on_doctype_update():
-	frappe.db.add_index("Purchase Invoice", ["supplier", "is_return", "return_against"])
-
-
 @frappe.whitelist()
 def make_purchase_receipt(source_name, target_doc=None):
 	def update_item(obj, target, source_parent):

erpnext/accounts/doctype/sales_invoice/sales_invoice.py

@@ -2549,10 +2549,6 @@ def get_loyalty_programs(customer):
 		return lp_details
 
 
-def on_doctype_update():
-	frappe.db.add_index("Sales Invoice", ["customer", "is_return", "return_against"])
-
-
 @frappe.whitelist()
 def create_invoice_discounting(source_name, target_doc=None):
 	invoice = frappe.get_doc("Sales Invoice", source_name)

erpnext/buying/doctype/purchase_order_item/purchase_order_item.json

@@ -123,8 +123,7 @@
    "oldfieldname": "item_code",
    "oldfieldtype": "Link",
    "options": "Item",
-   "reqd": 1,
+   "reqd": 1
-   "search_index": 1
   },
   {
    "fieldname": "supplier_part_no",

erpnext/patches.txt

@@ -353,5 +353,5 @@ execute:frappe.db.set_single_value("Buying Settings", "project_update_frequency"
 erpnext.patches.v14_0.update_total_asset_cost_field
 # below migration patch should always run last
 erpnext.patches.v14_0.migrate_gl_to_payment_ledger
-erpnext.stock.doctype.delivery_note.patches.drop_unused_return_against_index
+erpnext.stock.doctype.delivery_note.patches.drop_unused_return_against_index # 2023-12-20
-erpnext.patches.v14_0.set_maintain_stock_for_bom_item
+erpnext.patches.v14_0.set_maintain_stock_for_bom_item

erpnext/stock/doctype/bin/bin.json

@@ -52,8 +52,7 @@
    "oldfieldtype": "Link",
    "options": "Item",
    "read_only": 1,
-   "reqd": 1,
+   "reqd": 1
-   "search_index": 1
   },
   {
    "default": "0.00",

erpnext/stock/doctype/delivery_note/patches/drop_unused_return_against_index.py

@@ -1,15 +1,27 @@
+import click
 import frappe
 
+UNUSED_INDEXES = [
+	("Delivery Note", ["customer", "is_return", "return_against"]),
+	("Sales Invoice", ["customer", "is_return", "return_against"]),
+	("Purchase Invoice", ["supplier", "is_return", "return_against"]),
+	("Purchase Receipt", ["supplier", "is_return", "return_against"]),
+]
+
 
 def execute():
-	"""Drop unused return_against index"""
+	for doctype, index_fields in UNUSED_INDEXES:
+		table = f"tab{doctype}"
+		index_name = frappe.db.get_index_name(index_fields)
+		drop_index_if_exists(table, index_name)
+
+
+def drop_index_if_exists(table: str, index: str):
+	if not frappe.db.has_index(table, index):
+		return
 
 	try:
-		frappe.db.sql_ddl(
+		frappe.db.sql_ddl(f"ALTER TABLE `{table}` DROP INDEX `{index}`")
-			"ALTER TABLE `tabDelivery Note` DROP INDEX `customer_is_return_return_against_index`"
+		click.echo(f"✓ dropped {index} index from {table}")
-		)
-		frappe.db.sql_ddl(
-			"ALTER TABLE `tabPurchase Receipt` DROP INDEX `supplier_is_return_return_against_index`"
-		)
 	except Exception:
-		frappe.log_error("Failed to drop unused index")
+		frappe.log_error("Failed to drop index")

erpnext/tests/test_perf.py

@@ -0,0 +1,24 @@
+import frappe
+from frappe.tests.utils import FrappeTestCase
+
+INDEXED_FIELDS = {
+	"Bin": ["item_code"],
+	"GL Entry": ["voucher_type", "against_voucher_type"],
+	"Purchase Order Item": ["item_code"],
+	"Stock Ledger Entry": ["warehouse"],
+}
+
+
+class TestPerformance(FrappeTestCase):
+	def test_ensure_indexes(self):
+		# These fields are not explicitly indexed BUT they are prefix in some
+		# other composite index. If those are removed this test should be
+		# updated accordingly.
+		for doctype, fields in INDEXED_FIELDS.items():
+			for field in fields:
+				self.assertTrue(
+					frappe.db.sql(
+						f"""SHOW INDEX FROM `tab{doctype}`
+						WHERE Column_name = "{field}" AND Seq_in_index = 1"""
+					)
+				)