refactor!: remove implicit primary key from logs (frappe#22209)

frappe/core/doctype/doctype/doctype.py

@@ -539,7 +539,7 @@ def setup_autoincrement_and_sequence(self):
 
 		if self.autoname == "autoincrement":
 			name_type = "bigint"
-			frappe.db.create_sequence(self.name, check_not_exists=True, cache=frappe.db.SEQUENCE_CACHE)
+			frappe.db.create_sequence(self.name, check_not_exists=True)
 
 		change_name_column_type(self.name, name_type)
 

frappe/database/database.py

@@ -59,23 +59,6 @@ class Database:
 	CHILD_TABLE_COLUMNS = ("parent", "parenttype", "parentfield")
 	MAX_WRITES_PER_TRANSACTION = 200_000
 
-	# NOTE:
-	# FOR MARIADB - using no cache - as during backup, if the sequence was used in anyform,
-	# it drops the cache and uses the next non cached value in setval query and
-	# puts that in the backup file, which will start the counter
-	# from that value when inserting any new record in the doctype.
-	# By default the cache is 1000 which will mess up the sequence when
-	# using the system after a restore.
-	#
-	# Another case could be if the cached values expire then also there is a chance of
-	# the cache being skipped.
-	#
-	# FOR POSTGRES - The sequence cache for postgres is per connection.
-	# Since we're opening and closing connections for every request this results in skipping the cache
-	# to the next non-cached value hence not using cache in postgres.
-	# ref: https://stackoverflow.com/questions/21356375/postgres-9-0-4-sequence-skipping-numbers
-	SEQUENCE_CACHE = 0
-
 	class InvalidColumnName(frappe.ValidationError):
 		pass
 

frappe/database/mariadb/schema.py

@@ -3,7 +3,6 @@
 import frappe
 from frappe import _
 from frappe.database.schema import DBTable
-from frappe.model import log_types
 
 
 class MariaDBTable(DBTable):
@@ -36,11 +35,8 @@ def create(self):
 			additional_definitions.append("index modified(modified)")
 
 		# creating sequence(s)
-		if (
-			not self.meta.issingle and self.meta.autoname == "autoincrement"
-		) or self.doctype in log_types:
-
-			frappe.db.create_sequence(self.doctype, check_not_exists=True, cache=frappe.db.SEQUENCE_CACHE)
+		if not self.meta.issingle and self.meta.autoname == "autoincrement":
+			frappe.db.create_sequence(self.doctype, check_not_exists=True)
 
 			# NOTE: not used nextval func as default as the ability to restore
 			# database with sequences has bugs in mariadb and gives a scary error.

frappe/database/postgres/schema.py

@@ -1,7 +1,6 @@
 import frappe
 from frappe import _
 from frappe.database.schema import DBTable, get_definition
-from frappe.model import log_types
 from frappe.utils import cint, flt
 
 
@@ -30,11 +29,8 @@ def create(self):
 			)
 
 		# creating sequence(s)
-		if (
-			not self.meta.issingle and self.meta.autoname == "autoincrement"
-		) or self.doctype in log_types:
-
-			frappe.db.create_sequence(self.doctype, check_not_exists=True, cache=frappe.db.SEQUENCE_CACHE)
+		if not self.meta.issingle and self.meta.autoname == "autoincrement":
+			frappe.db.create_sequence(self.doctype, check_not_exists=True)
 			name_column = "name bigint primary key"
 
 		# TODO: set docstatus length

frappe/database/sequence.py

@@ -1,5 +1,22 @@
 from frappe import db, scrub
 
+# NOTE:
+# FOR MARIADB - using no cache - as during backup, if the sequence was used in anyform,
+# it drops the cache and uses the next non cached value in setval query and
+# puts that in the backup file, which will start the counter
+# from that value when inserting any new record in the doctype.
+# By default the cache is 1000 which will mess up the sequence when
+# using the system after a restore.
+#
+# Another case could be if the cached values expire then also there is a chance of
+# the cache being skipped.
+#
+# FOR POSTGRES - The sequence cache for postgres is per connection.
+# Since we're opening and closing connections for every request this results in skipping the cache
+# to the next non-cached value hence not using cache in postgres.
+# ref: https://stackoverflow.com/questions/21356375/postgres-9-0-4-sequence-skipping-numbers
+SEQUENCE_CACHE = 0
+
 
 def create_sequence(
 	doctype_name: str,
@@ -8,7 +25,7 @@ def create_sequence(
 	temporary: bool = False,
 	check_not_exists: bool = False,
 	cycle: bool = False,
-	cache: int = 0,
+	cache: int = SEQUENCE_CACHE,
 	start_value: int = 0,
 	increment_by: int = 0,
 	min_value: int = 0,

frappe/model/__init__.py

@@ -115,8 +115,6 @@
 	"Client Script",
 )
 
-# NOTE: this is being used for dynamic autoincrement in new sites,
-# removing any of these will require patches.
 log_types = (
 	"Version",
 	"Error Log",

frappe/model/naming.py

@@ -3,7 +3,6 @@
 
 import datetime
 import re
-from collections import defaultdict
 from collections.abc import Callable
 from typing import TYPE_CHECKING, Optional
 
@@ -12,7 +11,6 @@
 from frappe.model import log_types
 from frappe.query_builder import DocType
 from frappe.utils import cint, cstr, now_datetime
-from frappe.utils.caching import redis_cache
 
 if TYPE_CHECKING:
 	from frappe.model.document import Document
@@ -177,28 +175,15 @@ def set_new_name(doc):
 def is_autoincremented(doctype: str, meta: Optional["Meta"] = None) -> bool:
 	"""Checks if the doctype has autoincrement autoname set"""
 
-	if doctype in log_types:
-		return _implicitly_auto_incremented(doctype)
-	else:
-		if not meta:
-			meta = frappe.get_meta(doctype)
+	if not meta:
+		meta = frappe.get_meta(doctype)
 
-		if not getattr(meta, "issingle", False) and meta.autoname == "autoincrement":
-			return True
+	if not getattr(meta, "issingle", False) and meta.autoname == "autoincrement":
+		return True
 
 	return False
 
 
-@redis_cache
-def _implicitly_auto_incremented(doctype) -> bool:
-	query = f"""select data_type FROM information_schema.columns where column_name = 'name' and table_name = 'tab{doctype}'"""
-	values = ()
-	if frappe.db.db_type == "mariadb":
-		query += " and table_schema = %s"
-		values = (frappe.db.db_name,)
-	return frappe.db.sql(query, values)[0][0] == "bigint"
-
-
 def set_name_from_naming_options(autoname, doc):
 	"""
 	Get a name based on the autoname field option

frappe/patches.txt

@@ -1,4 +1,5 @@
 [pre_model_sync]
+frappe.patches.v15_0.remove_implicit_primary_key
 frappe.patches.v12_0.remove_deprecated_fields_from_doctype #3
 execute:frappe.utils.global_search.setup_global_search_table()
 execute:frappe.reload_doc('core', 'doctype', 'doctype_action', force=True) #2019-09-23

frappe/patches/v15_0/remove_implicit_primary_key.py

@@ -0,0 +1,46 @@
+import frappe
+from frappe.model.naming import is_autoincremented
+
+possible_log_types = (
+	"Version",
+	"Error Log",
+	"Scheduled Job Log",
+	"Event Sync Log",
+	"Event Update Log",
+	"Access Log",
+	"View Log",
+	"Activity Log",
+	"Energy Point Log",
+	"Notification Log",
+	"Email Queue",
+	"DocShare",
+	"Document Follow",
+	"Console Log",
+)
+
+
+def execute():
+	"""Few doctypes had int PKs even though schema didn't mention them, this requires detecting it
+	at runtime which is prone to bugs and adds unnecessary overhead.
+
+	This patch converts them back to varchar.
+	"""
+	for doctype in possible_log_types:
+		if _is_implicit_int_pk(doctype) and not is_autoincremented(doctype):
+			frappe.db.change_column_type(
+				doctype,
+				"name",
+				type=f"varchar({frappe.db.VARCHAR_LEN})",
+				nullable=True,
+			)
+
+
+def _is_implicit_int_pk(doctype: str) -> bool:
+	query = f"""select data_type FROM information_schema.columns where column_name = 'name' and table_name = 'tab{doctype}'"""
+	values = ()
+	if frappe.db.db_type == "mariadb":
+		query += " and table_schema = %s"
+		values = (frappe.db.db_name,)
+
+	col_type = frappe.db.sql(query, values)
+	return bool(col_type and col_type[0][0] == "bigint")