{170477625}: Fixing dbstore function

Require a dbstore function to also be nullable. The dbstore function, however,
will be evaluated for new records only. See discussion in bloomberg#3781.

Signed-off-by: Rivers Zhang <[email protected]>

db/sqlglue.c

@@ -13192,4 +13192,3 @@ int comdb2_is_field_indexable(const char *table_name, int fld_idx) {
     }
     return 1;
 }
-

db/tag.c

@@ -3321,11 +3321,14 @@ static int stag_to_stag_field(const char *inbuf, char *outbuf, int flags,
         } else {
             if (fail_reason)
                 fail_reason->source_field_idx = field_idx;
-            rc = SERVER_to_SERVER(
-                to_field->in_default, to_field->in_default_len,
-                to_field->in_default_type, NULL /*convopts*/, NULL /*blob*/, 0,
-                outbuf + to_field->offset, to_field->len, to_field->type,
-                oflags, &outdtsz, &to_field->convopts, outblob /*blob*/
+            /* Be sure to evalute a dbstore function for new records only.
+               For existing records, use NULL */
+            if (to_field->in_default_type == SERVER_FUNCTION)
+                rc = NULL_to_SERVER(outbuf + to_field->offset, to_field->len, to_field->type);
+            else
+                rc = SERVER_to_SERVER(to_field->in_default, to_field->in_default_len, to_field->in_default_type,
+                                      NULL /*convopts*/, NULL /*blob*/, 0, outbuf + to_field->offset, to_field->len,
+                                      to_field->type, oflags, &outdtsz, &to_field->convopts, outblob /*blob*/
                 );
             if (rc) {
                 if (fail_reason)
@@ -3926,6 +3929,14 @@ int compare_tag_int(struct schema *old, struct schema *new, FILE *out,
                         }
                         return SC_BAD_NEW_FIELD;
                     }
+
+                    if (fnew->in_default_type == SERVER_FUNCTION && fold->in_default_type != SERVER_FUNCTION &&
+                        (fnew->flags & NO_NULL)) {
+                        if (out)
+                            logmsg(LOGMSG_ERROR, "field %s must be nullable to set a default function\n", fold->name);
+                        return SC_BAD_DBSTORE_FUNC_NOT_NULL;
+                    }
+
                 } else {
                     assert(fold->in_default_len == fnew->in_default_len);
                     int len = fold->in_default_len;
@@ -4066,12 +4077,20 @@ int compare_tag_int(struct schema *old, struct schema *new, FILE *out,
                             old->tag, nidx, fnew->name);
                 }
                 break;
-            } else if (allow_null || (fnew->in_default && fnew->in_default_type != SERVER_SEQUENCE)) {
+            } else if (allow_null || (fnew->in_default && fnew->in_default_type != SERVER_SEQUENCE &&
+                                      fnew->in_default_type != SERVER_FUNCTION)) {
                 rc = SC_COLUMN_ADDED;
                 if (out) {
                     logmsg(LOGMSG_INFO, "tag %s has new field %d (named %s)\n",
                             old->tag, nidx, fnew->name);
                 }
+            } else if (!allow_null && fnew->in_default_type == SERVER_FUNCTION) {
+                rc = SC_BAD_DBSTORE_FUNC_NOT_NULL;
+                if (out) {
+                    logmsg(LOGMSG_INFO, "tag %s has new field %d (named %s)"
+                           "that uses a dbstore function but isn't nullable\n",
+                           old->tag, nidx, fnew->name);
+                }
             } else {
                 if (out) {
                     logmsg(LOGMSG_INFO, "tag %s has new field %d (named %s) without "

db/tag.h

@@ -194,9 +194,12 @@ enum {
     SC_BAD_NEW_FIELD = -3,
     SC_BAD_INDEX_CHANGE = -4,
     SC_BAD_INDEX_NAME = -5,
-    SC_BAD_DBPAD = -6
+    SC_BAD_DBPAD = -6,
+    SC_BAD_DBSTORE_FUNC_NOT_NULL = -7,
 };
 
+#define DBPAD_OR_DBSTORE_ERR(e) ((e) == SC_BAD_NEW_FIELD || (e) == SC_BAD_DBPAD || (e) == SC_BAD_DBSTORE_FUNC_NOT_NULL)
+
 extern hash_t *gbl_tag_hash;
 extern char gbl_ver_temp_table[];
 extern char gbl_ondisk_ver[];

schemachange/sc_alter_table.c

@@ -66,7 +66,7 @@ static int prepare_changes(struct schema_change_type *s, struct dbtable *db,
 {
     int changed = ondisk_schema_changed(s->tablename, newdb, stderr, s);
 
-    if (changed == SC_BAD_NEW_FIELD || changed == SC_BAD_DBPAD) {
+    if (DBPAD_OR_DBSTORE_ERR(changed)) {
         /* we want to capture cases when "alter" is used
            to add a field to a table that has no dbstore or
            isnull specified,
@@ -90,6 +90,8 @@ static int prepare_changes(struct schema_change_type *s, struct dbtable *db,
             sc_client_error(s, "cannot change index referenced by other tables");
         } else if (changed == SC_BAD_DBPAD) {
             sc_client_error(s, "cannot change size of byte array without dbpad");
+        } else if (changed == SC_BAD_DBSTORE_FUNC_NOT_NULL) {
+            sc_client_error(s, "column must be nullable to use a function as its default value");
         }
         sc_errf(s, "Failed to process schema!\n");
         return -1;

schemachange/sc_logic.c

@@ -1498,6 +1498,9 @@ int dryrun_int(struct schema_change_type *s, struct dbtable *db, struct dbtable
         } else if (changed == SC_BAD_DBPAD) {
             sc_client_error(s, ">Cannot change size of byte array without dbpad\n");
             return -1;
+        } else if (changed == SC_BAD_DBSTORE_FUNC_NOT_NULL) {
+            sc_client_error(s, ">Column must be nullable to use a function as its default value");
+            return -1;
         } else {
             sc_client_error(s, ">Failed to process schema!\n");
             return -1;

schemachange/sc_struct.c

@@ -1053,7 +1053,7 @@ static int reload_csc2_schema(struct dbtable *db, tran_type *tran,
     changed = ondisk_schema_changed(table, newdb, NULL, NULL);
     /* let this fly, which will be ok for fastinit;
        master will catch early non-fastinit cases */
-    if (changed < 0 && changed != SC_BAD_NEW_FIELD && changed != SC_BAD_DBPAD) {
+    if (changed < 0 && !DBPAD_OR_DBSTORE_ERR(changed)) {
         if (changed == -2) {
             logmsg(LOGMSG_ERROR, "Error reloading schema!\n");
         }

tests/sc_dbstore_func.test/Makefile

@@ -0,0 +1,5 @@
+ifeq ($(TESTSROOTDIR),)
+  include ../testcase.mk
+else
+  include $(TESTSROOTDIR)/testcase.mk
+endif

tests/sc_dbstore_func.test/output.expected

@@ -0,0 +1,16 @@
+- Testing dbstore now() -
+1
+-- The schema change below should fail --
+[ALTER TABLE t1 { tag ondisk { int i datetime t dbstore={now()} } }] failed with rc 240 column must be nullable to use a function as its default value
+-- The schema change below should succeed, instantly --
+1
+-- Verify records --
+1
+-- Verify records again after rebuild --
+1
+-- The schema change below should succeed. dta is untouched but a new index is built --
+1
+-- Verify records --
+1
+-- Verify records again after full rebuild --
+1

tests/sc_dbstore_func.test/runit

@@ -0,0 +1,30 @@
+#!/usr/bin/env bash
+bash -n "$0" | exit 1
+
+dbnm=$1
+
+cat << EOF | cdb2sql ${CDB2_OPTIONS} -tabs -s $dbnm default - >output.actual 2>&1
+SELECT "- Testing dbstore now() -"
+CREATE TABLE t1 { tag ondisk { int i } }\$\$
+INSERT INTO t1 VALUES (1)
+SELECT "-- The schema change below should fail --"
+ALTER TABLE t1 { tag ondisk { int i datetime t dbstore={now()} } }\$\$
+SELECT "-- The schema change below should succeed, instantly --"
+ALTER TABLE t1 { tag ondisk { int i datetime t dbstore={now()} null=yes } }\$\$
+INSERT INTO t1(i) VALUES (1)
+SELECT "-- Verify records --"
+SELECT COUNT(*) FROM t1 WHERE t IS NOT NULL
+SELECT "-- Verify records again after rebuild --"
+REBUILD t1
+SELECT COUNT(*) FROM t1 WHERE t IS NOT NULL
+SELECT "-- The schema change below should succeed. dta is untouched but a new index is built --"
+ALTER TABLE t1 { tag ondisk { int i datetime t dbstore={now()} null=yes byte b[16] dbstore={guid()} null=yes } keys { uniqnulls "KEY_B" = b } }\$\$
+INSERT INTO t1(i) VALUES (1)
+SELECT "-- Verify records --"
+SELECT COUNT(*) FROM t1 WHERE b IS NOT NULL
+SELECT "-- Verify records again after full rebuild --"
+REBUILD t1
+SELECT COUNT(*) FROM t1 WHERE b IS NOT NULL
+EOF
+
+diff output.actual output.expected