[tarantool-patches] [PATCH] sql: remove preupdate hook

[tarantool-patches] [PATCH] sql: remove preupdate hook

[Kirill Yukhin]

SQLITE_ENABLE_PREUPDATE_HOOK is dead macro.
Remove it.

Part of #2356
---
Issue (pretty much irrelevant): https://github.com/tarantool/tarantool/commits/kyukhin/gh-2356-remove-preupdate-hook
Branch: https://github.com/tarantool/tarantool/issues/2356

 src/box/sql/main.c      | 20 --------------------
 src/box/sql/sqliteInt.h | 12 ------------
 src/box/sql/update.c    |  8 --------
 src/box/sql/vdbe.c      | 10 ----------
 src/box/sql/vdbeapi.c   | 20 --------------------
 src/box/sql/vdbeaux.c   | 25 -------------------------
 6 files changed, 95 deletions(-)

diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index ded3b5b..4187bb5 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1325,26 +1325,6 @@ sqlite3_rollback_hook(sqlite3 * db,	/* Attach the hook to this database */
 	return pRet;
 }
 
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-/*
- * Register a callback to be invoked each time a row is updated,
- * inserted or deleted using this database connection.
- */
-void *
-sqlite3_preupdate_hook(sqlite3 * db,		/* Attach the hook to this database */
-		       void (*xCallback) (	/* Callback function */
-			       void *, sqlite3 *, int,
-			       char const *, sqlite3_int64, sqlite3_int64),
-		       void *pArg)		/* First callback argument */
-{
-	void *pRet;
-	pRet = db->pPreUpdateArg;
-	db->xPreUpdateCallback = xCallback;
-	db->pPreUpdateArg = pArg;
-	return pRet;
-}
-#endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
-
 /*
  * Configure an sqlite3_wal_hook() callback to automatically checkpoint
  * a database after committing a transaction if there are nFrame or
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 5ca91b2..5ee5606 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1626,14 +1626,6 @@ struct sqlite3 {
 	void *pUpdateArg;
 	void (*xUpdateCallback) (void *, int, const char *, const char *,
 				 sqlite_int64);
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-	void *pPreUpdateArg;	/* First argument to xPreUpdateCallback */
-	void (*xPreUpdateCallback) (	/* Registered using sqlite3_preupdate_hook() */
-					   void *, sqlite3 *, int, char const *,
-					   char const *, sqlite3_int64,
-					   sqlite3_int64);
-	PreUpdate *pPreUpdate;	/* Context for active pre-update callback */
-#endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
 	sqlite3_value *pErr;	/* Most recent error message */
 	union {
 		volatile int isInterrupted;	/* True if sqlite3_interrupt has been called */
@@ -2956,12 +2948,8 @@ struct Parse {
 #define OPFLAG_NCHANGE       0x01	/* OP_Insert: Set to update db->nChange */
 				     /* Also used in P2 (not P5) of OP_Delete */
 #define OPFLAG_EPHEM         0x01	/* OP_Column: Ephemeral output is ok */
-#define OPFLAG_ISUPDATE      0x04	/* This OP_Insert is an sql UPDATE */
 #define OPFLAG_OE_IGNORE    0x200	/* OP_IdxInsert: Ignore flag */
 #define OPFLAG_OE_FAIL      0x400	/* OP_IdxInsert: Fail flag */
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-#define OPFLAG_ISNOOP        0x40	/* OP_Delete does pre-update-hook only */
-#endif
 #define OPFLAG_LENGTHARG     0x40	/* OP_Column only used for length() */
 #define OPFLAG_TYPEOFARG     0x80	/* OP_Column only used for typeof() */
 #define OPFLAG_SEEKEQ        0x02	/* OP_Open** cursor uses EQ seek only */
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d51a05c..29a9bd4 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -568,17 +568,9 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		 * is the column index supplied by the user.
 		 */
 		assert(regNew == regNewPk + 1);
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-		sqlite3VdbeAddOp3(v, OP_Delete, iDataCur,
-				  OPFLAG_ISUPDATE |
-				  ((hasFK || chngKey
-				    || pPk != 0) ? 0 : OPFLAG_ISNOOP),
-				  regNewRowid);
-#else
 		if (hasFK || chngPk || pPk != 0) {
 			sqlite3VdbeAddOp2(v, OP_Delete, iDataCur, 0);
 		}
-#endif
 		if (bReplace || chngPk) {
 			sqlite3VdbeJumpHere(v, addr1);
 		}
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index dc22c26..2144d95 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -119,16 +119,6 @@ updateMaxBlobsize(Mem *p)
 }
 #endif
 
-/*
- * This macro evaluates to true if either the update hook or the preupdate
- * hook are enabled for database connect DB.
- */
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-# define HAS_UPDATE_HOOK(DB) ((DB)->xPreUpdateCallback||(DB)->xUpdateCallback)
-#else
-# define HAS_UPDATE_HOOK(DB) ((DB)->xUpdateCallback)
-#endif
-
 /*
  * The next global variable is incremented each time the OP_Found opcode
  * is executed. This is used to test whether or not the foreign key
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index f1068d3..d3a91e2 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -1574,26 +1574,6 @@ sqlite3_expanded_sql(sqlite3_stmt * pStmt)
 #endif
 }
 
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-/*
- * This function is designed to be called from within a pre-update callback
- * only. It returns zero if the change that caused the callback was made
- * immediately by a user SQL statement. Or, if the change was made by a
- * trigger program, it returns the number of trigger programs currently
- * on the stack (1 for a top-level trigger, 2 for a trigger fired by a
- * top-level trigger etc.).
- *
- * For the purposes of the previous paragraph, a foreign key CASCADE, SET NULL
- * or SET DEFAULT action is considered a trigger.
- */
-int
-sqlite3_preupdate_depth(sqlite3 * db)
-{
-	PreUpdate *p = db->pPreUpdate;
-	return (p ? p->v->nFrame : 0);
-}
-#endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
-
 #ifdef SQLITE_ENABLE_STMT_SCANSTATUS
 /*
  * Return status data for a single loop within query pStmt.
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index c71e0fe..d6ff51c 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -3625,31 +3625,6 @@ sqlite3VdbeSetVarmask(Vdbe * v, int iVar)
 	}
 }
 
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-
-/*
- * If the second argument is not NULL, release any allocations associated
- * with the memory cells in the p->aMem[] array. Also free the UnpackedRecord
- * structure itself, using sqlite3DbFree().
- *
- * This function is used to free UnpackedRecord structures allocated by
- * the vdbeUnpackRecord() function found in vdbeapi.c.
- */
-static void
-vdbeFreeUnpacked(sqlite3 * db, UnpackedRecord * p)
-{
-	if (p) {
-		int i;
-		for (i = 0; i < p->nField; i++) {
-			Mem *pMem = &p->aMem[i];
-			if (pMem->zMalloc)
-				sqlite3VdbeMemRelease(pMem);
-		}
-		sqlite3DbFree(db, p);
-	}
-}
-#endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
-
 i64
 sqlite3VdbeMsgpackRecordLen(Mem * pRec, u32 n)
 {
-- 
2.16.2

[tarantool-patches] Re: [PATCH] sql: remove preupdate hook

[n.pettik]

I have grepped and found few mentions about preupdate hook
which you didn’t delete.

Firstly, it seems that you can remove struct PreUpdate.
Secondly, one comment in update.c (line 565):

* That (regNew==regnewPk+1) is true is also important for the
* pre-update hook. If the caller invokes preupdate_new(), the returned
* value is copied from memory cell (regNewPk+1+iCol), where iCol
* is the column index supplied by the user.

Remove it as well pls.

[tarantool-patches] Re: [PATCH] sql: remove preupdate hook

[Kirill Yukhin]

Hello Nikita,
On 27 июл 14:40, n.pettik wrote:
> I have grepped and found few mentions about preupdate hook
> which you didn’t delete.
> 
> Firstly, it seems that you can remove struct PreUpdate.
> Secondly, one comment in update.c (line 565):
> 
> * That (regNew==regnewPk+1) is true is also important for the
> * pre-update hook. If the caller invokes preupdate_new(), the returned
> * value is copied from memory cell (regNewPk+1+iCol), where iCol
> * is the column index supplied by the user.
> 
> Remove it as well pls.
Fixed. Updated patch below.

--
Regards, Kirill Yukhin

commit 5d03ba58982a89b8b1eb0f86579e37a213e5b3af
Author: Kirill Yukhin <kyukhin@tarantool.org>
Date:   Fri Jul 27 14:00:06 2018 +0300

    sql: remove preupdate hook
    
    SQLITE_ENABLE_PREUPDATE_HOOK is dead macro.
    Remove it.
    
    Part of #2356

diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index ded3b5b..4187bb5 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1325,26 +1325,6 @@ sqlite3_rollback_hook(sqlite3 * db,	/* Attach the hook to this database */
 	return pRet;
 }
 
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-/*
- * Register a callback to be invoked each time a row is updated,
- * inserted or deleted using this database connection.
- */
-void *
-sqlite3_preupdate_hook(sqlite3 * db,		/* Attach the hook to this database */
-		       void (*xCallback) (	/* Callback function */
-			       void *, sqlite3 *, int,
-			       char const *, sqlite3_int64, sqlite3_int64),
-		       void *pArg)		/* First callback argument */
-{
-	void *pRet;
-	pRet = db->pPreUpdateArg;
-	db->xPreUpdateCallback = xCallback;
-	db->pPreUpdateArg = pArg;
-	return pRet;
-}
-#endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
-
 /*
  * Configure an sqlite3_wal_hook() callback to automatically checkpoint
  * a database after committing a transaction if there are nFrame or
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 5ca91b2..b595984 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1483,7 +1483,6 @@ typedef struct Lookaside Lookaside;
 typedef struct LookasideSlot LookasideSlot;
 typedef struct NameContext NameContext;
 typedef struct Parse Parse;
-typedef struct PreUpdate PreUpdate;
 typedef struct PrintfArguments PrintfArguments;
 typedef struct RowSet RowSet;
 typedef struct Savepoint Savepoint;
@@ -1626,14 +1625,6 @@ struct sqlite3 {
 	void *pUpdateArg;
 	void (*xUpdateCallback) (void *, int, const char *, const char *,
 				 sqlite_int64);
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-	void *pPreUpdateArg;	/* First argument to xPreUpdateCallback */
-	void (*xPreUpdateCallback) (	/* Registered using sqlite3_preupdate_hook() */
-					   void *, sqlite3 *, int, char const *,
-					   char const *, sqlite3_int64,
-					   sqlite3_int64);
-	PreUpdate *pPreUpdate;	/* Context for active pre-update callback */
-#endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
 	sqlite3_value *pErr;	/* Most recent error message */
 	union {
 		volatile int isInterrupted;	/* True if sqlite3_interrupt has been called */
@@ -2956,12 +2947,8 @@ struct Parse {
 #define OPFLAG_NCHANGE       0x01	/* OP_Insert: Set to update db->nChange */
 				     /* Also used in P2 (not P5) of OP_Delete */
 #define OPFLAG_EPHEM         0x01	/* OP_Column: Ephemeral output is ok */
-#define OPFLAG_ISUPDATE      0x04	/* This OP_Insert is an sql UPDATE */
 #define OPFLAG_OE_IGNORE    0x200	/* OP_IdxInsert: Ignore flag */
 #define OPFLAG_OE_FAIL      0x400	/* OP_IdxInsert: Fail flag */
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-#define OPFLAG_ISNOOP        0x40	/* OP_Delete does pre-update-hook only */
-#endif
 #define OPFLAG_LENGTHARG     0x40	/* OP_Column only used for length() */
 #define OPFLAG_TYPEOFARG     0x80	/* OP_Column only used for typeof() */
 #define OPFLAG_SEEKEQ        0x02	/* OP_Open** cursor uses EQ seek only */
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d51a05c..489a456 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -559,26 +559,12 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		}
 
 		/* If changing the PK value, or if there are foreign key constraints
-		 * to process, delete the old record. Otherwise, add a noop OP_Delete
-		 * to invoke the pre-update hook.
-		 *
-		 * That (regNew==regnewPk+1) is true is also important for the
-		 * pre-update hook. If the caller invokes preupdate_new(), the returned
-		 * value is copied from memory cell (regNewPk+1+iCol), where iCol
-		 * is the column index supplied by the user.
+		 * to process, delete the old record.
 		 */
 		assert(regNew == regNewPk + 1);
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-		sqlite3VdbeAddOp3(v, OP_Delete, iDataCur,
-				  OPFLAG_ISUPDATE |
-				  ((hasFK || chngKey
-				    || pPk != 0) ? 0 : OPFLAG_ISNOOP),
-				  regNewRowid);
-#else
 		if (hasFK || chngPk || pPk != 0) {
 			sqlite3VdbeAddOp2(v, OP_Delete, iDataCur, 0);
 		}
-#endif
 		if (bReplace || chngPk) {
 			sqlite3VdbeJumpHere(v, addr1);
 		}
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index dc22c26..2144d95 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -119,16 +119,6 @@ updateMaxBlobsize(Mem *p)
 }
 #endif
 
-/*
- * This macro evaluates to true if either the update hook or the preupdate
- * hook are enabled for database connect DB.
- */
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-# define HAS_UPDATE_HOOK(DB) ((DB)->xPreUpdateCallback||(DB)->xUpdateCallback)
-#else
-# define HAS_UPDATE_HOOK(DB) ((DB)->xUpdateCallback)
-#endif
-
 /*
  * The next global variable is incremented each time the OP_Found opcode
  * is executed. This is used to test whether or not the foreign key
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index e5ed94c..a4770a5 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -420,24 +420,6 @@ struct Vdbe {
 #define VDBE_MAGIC_RESET    0x48fa9f76	/* Reset and ready to run again */
 #define VDBE_MAGIC_DEAD     0x5606c3c8	/* The VDBE has been deallocated */
 
-/*
- * Structure used to store the context required by the
- * sqlite3_preupdate_*() API functions.
- */
-struct PreUpdate {
-	Vdbe *v;
-	VdbeCursor *pCsr;	/* Cursor to read old values from */
-	int op;			/* One of SQLITE_INSERT, UPDATE, DELETE */
-	u8 *aRecord;		/* old.* database record */
-	UnpackedRecord *pUnpacked;	/* Unpacked version of aRecord[] */
-	UnpackedRecord *pNewUnpacked;	/* Unpacked version of new.* record */
-	int iNewReg;		/* Register for new.* values */
-	i64 iKey1;		/* First key value passed to hook */
-	i64 iKey2;		/* Second key value passed to hook */
-	Mem *aNew;		/* Array of new.* values */
-	Table *pTab;		/* Schema object being upated */
-};
-
 /*
  * Function prototypes
  */
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index f1068d3..d3a91e2 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -1574,26 +1574,6 @@ sqlite3_expanded_sql(sqlite3_stmt * pStmt)
 #endif
 }
 
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-/*
- * This function is designed to be called from within a pre-update callback
- * only. It returns zero if the change that caused the callback was made
- * immediately by a user SQL statement. Or, if the change was made by a
- * trigger program, it returns the number of trigger programs currently
- * on the stack (1 for a top-level trigger, 2 for a trigger fired by a
- * top-level trigger etc.).
- *
- * For the purposes of the previous paragraph, a foreign key CASCADE, SET NULL
- * or SET DEFAULT action is considered a trigger.
- */
-int
-sqlite3_preupdate_depth(sqlite3 * db)
-{
-	PreUpdate *p = db->pPreUpdate;
-	return (p ? p->v->nFrame : 0);
-}
-#endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
-
 #ifdef SQLITE_ENABLE_STMT_SCANSTATUS
 /*
  * Return status data for a single loop within query pStmt.
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index c71e0fe..d6ff51c 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -3625,31 +3625,6 @@ sqlite3VdbeSetVarmask(Vdbe * v, int iVar)
 	}
 }
 
-#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
-
-/*
- * If the second argument is not NULL, release any allocations associated
- * with the memory cells in the p->aMem[] array. Also free the UnpackedRecord
- * structure itself, using sqlite3DbFree().
- *
- * This function is used to free UnpackedRecord structures allocated by
- * the vdbeUnpackRecord() function found in vdbeapi.c.
- */
-static void
-vdbeFreeUnpacked(sqlite3 * db, UnpackedRecord * p)
-{
-	if (p) {
-		int i;
-		for (i = 0; i < p->nField; i++) {
-			Mem *pMem = &p->aMem[i];
-			if (pMem->zMalloc)
-				sqlite3VdbeMemRelease(pMem);
-		}
-		sqlite3DbFree(db, p);
-	}
-}
-#endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
-
 i64
 sqlite3VdbeMsgpackRecordLen(Mem * pRec, u32 n)
 {

[tarantool-patches] Re: [PATCH] sql: remove preupdate hook

[n.pettik]

>> I have grepped and found few mentions about preupdate hook
>> which you didn’t delete.
>> 
>> Firstly, it seems that you can remove struct PreUpdate.
>> Secondly, one comment in update.c (line 565):
>> 
>> * That (regNew==regnewPk+1) is true is also important for the
>> * pre-update hook. If the caller invokes preupdate_new(), the returned
>> * value is copied from memory cell (regNewPk+1+iCol), where iCol
>> * is the column index supplied by the user.
>> 
>> Remove it as well pls.
> Fixed. Updated patch below.

Thx, now LGTM.

[tarantool-patches] Re: [PATCH] sql: remove preupdate hook

[Kirill Yukhin]

Hello,
On 27 июл 14:04, Kirill Yukhin wrote:
> SQLITE_ENABLE_PREUPDATE_HOOK is dead macro.
> Remove it.
> 
> Part of #2356
> ---
> Issue (pretty much irrelevant): https://github.com/tarantool/tarantool/commits/kyukhin/gh-2356-remove-preupdate-hook
> Branch: https://github.com/tarantool/tarantool/issues/2356
I've checked the patch into 2.0 branch.

--
Regards, Kirill Yukhin