ets:insert/2 insert order preserved when list with same keys for bag table

erts/emulator/beam/erl_db.c

@@ -1682,17 +1682,18 @@ static void* ets_insert_2_list_copy_term_list(DbTableMethod* meth,
 {
     void* db_term_list = NULL;
     void *term;
+    void *last_term;
     Eterm lst;
     for (lst = list; is_list(lst); lst = CDR(list_val(lst))) {
         term = meth->db_eterm_to_dbterm(compress,
                                         keypos,
                                         CAR(list_val(lst)));
         if (db_term_list != NULL) {
-            db_term_list =
-                meth->db_dbterm_list_prepend(db_term_list,
-                                             term);
+            last_term =
+                meth->db_dbterm_list_append(last_term, term);
         } else {
             db_term_list = term;
+            last_term = term;
         }
     }
 

erts/emulator/beam/erl_db_catree.c

@@ -220,7 +220,7 @@ DbTableMethod db_catree =
     db_lookup_dbterm_catree,
     db_finalize_dbterm_catree,
     db_eterm_to_dbterm_tree_common,
-    db_dbterm_list_prepend_tree_common,
+    db_dbterm_list_append_tree_common,
     db_dbterm_list_remove_first_tree_common,
     db_put_dbterm_catree,
     db_free_dbterm_tree_common,

erts/emulator/beam/erl_db_hash.c

@@ -726,7 +726,7 @@ db_lookup_dbterm_hash(Process *p, DbTable *tbl, Eterm key, Eterm obj,
 static void
 db_finalize_dbterm_hash(int cret, DbUpdateHandle* handle);
 static void* db_eterm_to_dbterm_hash(int compress, int keypos, Eterm obj);
-static void* db_dbterm_list_prepend_hash(void* list, void* db_term);
+static void* db_dbterm_list_append_hash(void* last_term, void* db_term);
 static void* db_dbterm_list_remove_first_hash(void** list);
 static int db_put_dbterm_hash(DbTable* tb,
                               void* obj,
@@ -866,7 +866,7 @@ DbTableMethod db_hash =
     db_lookup_dbterm_hash,
     db_finalize_dbterm_hash,
     db_eterm_to_dbterm_hash,
-    db_dbterm_list_prepend_hash,
+    db_dbterm_list_append_hash,
     db_dbterm_list_remove_first_hash,
     db_put_dbterm_hash,
     db_free_dbterm_hash,
@@ -4170,11 +4170,11 @@ static void* db_eterm_to_dbterm_hash(int compress, int keypos, Eterm obj)
     return term;
 }
 
-static void* db_dbterm_list_prepend_hash(void* list, void* db_term)
+static void* db_dbterm_list_append_hash(void* last_term, void* db_term)
 {
-    HashDbTerm* l = list;
+    HashDbTerm* l = last_term;
     HashDbTerm* t = db_term;
-    t->next = l;
+    l->next = t;
     return t;
 }
 

erts/emulator/beam/erl_db_tree.c

@@ -519,7 +519,7 @@ DbTableMethod db_tree =
     db_lookup_dbterm_tree,
     db_finalize_dbterm_tree,
     db_eterm_to_dbterm_tree_common,
-    db_dbterm_list_prepend_tree_common,
+    db_dbterm_list_append_tree_common,
     db_dbterm_list_remove_first_tree_common,
     db_put_dbterm_tree,
     db_free_dbterm_tree_common,
@@ -3533,11 +3533,11 @@ void* db_eterm_to_dbterm_tree_common(int compress, int keypos, Eterm obj)
     return term;
 }
 
-void* db_dbterm_list_prepend_tree_common(void *list, void *db_term)
+void* db_dbterm_list_append_tree_common(void *last_term, void *db_term)
 {
-    TreeDbTerm* l = list;
+    TreeDbTerm* l = last_term;
     TreeDbTerm* t = db_term;
-    t->left = l;
+    l->left = t;
     return t;
 }
 

erts/emulator/beam/erl_db_tree_util.h

@@ -172,7 +172,7 @@ void db_finalize_dbterm_tree_common(int cret,
                                     TreeDbTerm **root,
                                     DbTableTree *stack_container);
 void* db_eterm_to_dbterm_tree_common(int compress, int keypos, Eterm obj);
-void* db_dbterm_list_prepend_tree_common(void* list, void* db_term);
+void* db_dbterm_list_append_tree_common(void* last_term, void* db_term);
 void* db_dbterm_list_remove_first_tree_common(void **list);
 int db_put_dbterm_tree_common(DbTableCommon *tb, TreeDbTerm **root, TreeDbTerm *value_to_insert,
                               int key_clash_fail, DbTableTree *stack_container);

erts/emulator/beam/erl_db_util.h

@@ -237,7 +237,7 @@ typedef struct db_table_method
     ** not DB_ERROR_NONE, the object is removed from the table. */
     void (*db_finalize_dbterm)(int cret, DbUpdateHandle* handle);
     void* (*db_eterm_to_dbterm)(int compress, int keypos, Eterm obj);
-    void* (*db_dbterm_list_prepend)(void* list, void* db_term);
+    void* (*db_dbterm_list_append)(void* last_term, void* db_term);
     void* (*db_dbterm_list_remove_first)(void** list);
     int (*db_put_dbterm)(DbTable* tb, /* [in out] */
                          void* obj,

lib/stdlib/doc/src/ets.xml

@@ -48,7 +48,26 @@
       A <c>set</c> or <c>ordered_set</c> table can only have one object
       associated with each key. A <c>bag</c> or <c>duplicate_bag</c> table can
       have many objects associated with each key.</p>
-
+    <p>
+      Insert and lookup times in tables of type <c>set</c> are constant,
+      regardless of the table size. For table types <c>bag</c> and
+      <c>duplicate_bag</c> time is proportional to the number of objects with the
+      same key. Even seemingly unrelated keys may inflict linear search to be
+      skipped past while looking for the key of interest (due to hash collision).
+    </p>
+    <warning>
+      <p>
+	For tables of type <c>bag</c> and <c>duplicate_bag</c>, avoid inserting
+	an extensive amount of objects with the same key. It will hurt insert and
+	lookup performance as well as real time characteristics of the runtime
+	environment (hash bucket linear search do not yield).
+      </p>
+    </warning>
+    <p>
+      The <c>ordered_set</c> table type uses a binary search tree. Insert and
+      lookup times are proportional to the logarithm of the number of objects in
+      the table.
+    </p>
    <marker id="max_ets_tables"></marker>
    <note>
      <p>
@@ -814,6 +833,12 @@ Error: fun containing local Erlang function calls
               inserted object <em>compares equal</em> to the key of any object
               in the table, the old object is replaced.</p>
           </item>
+          <item>
+            <p>
+	      If the table type is <c>bag</c> and the object <em>matches</em>
+	      any whole object in the table, the object is not inserted.
+            </p>
+          </item>
           <item>
             <p>If the list contains more than one object with
               <em>matching</em> keys and the table type is <c>set</c>, one is
@@ -825,6 +850,27 @@ Error: fun containing local Erlang function calls
         <p>The entire operation is guaranteed to be
           <seeerl marker="#concurrency">atomic and isolated</seeerl>,
           even when a list of objects is inserted.</p>
+        <marker id="insert_list_order"></marker>
+        <p>
+	  For <c>bag</c> and <c>duplicate_bag</c>, objects in the list with
+	  identical keys will be inserted in list order (from head to tail). That
+	  is, a subsequent call to <seemfa marker="#lookup/2"><c>lookup(T,Key)</c></seemfa>
+	  will return them in that inserted order.
+        </p>
+	<note>
+	  <p>
+	    For <c>bag</c> the insertion order of indentical keys described above was
+	    accidentally reverted in OTP 23.0 and later fixed in OTP 25.3. That
+	    is, from OTP 23.0 up until OTP 25.3 the objects in a list are
+	    inserted in reverse order (from tail to head).
+          </p>
+	  <p>
+	    For <c>duplicate_bag</c> the same faulty reverse insertion exist
+	    from OTP 23.0 until OTP 25.3. However, it is unpredictable and may
+	    or may not happen. A longer list will increase the probabiliy of the
+	    insertion being done in reverse.
+          </p>
+        </note>
       </desc>
     </func>
 
@@ -919,14 +965,11 @@ Error: fun containing local Erlang function calls
           element, as there cannot be more than one object with the same
           key. For tables of type <c>bag</c> or <c>duplicate_bag</c>, the
           function returns a list of arbitrary length.</p>
-        <p>Notice that the time order of object insertions is preserved;
+        <p>Notice that the sequential order of object insertions is preserved;
           the first object inserted with the specified key is the first
-          in the resulting list, and so on.</p>
-        <p>Insert and lookup times in tables of type <c>set</c>,
-          <c>bag</c>, and <c>duplicate_bag</c> are constant, regardless
-          of the table size. For the <c>ordered_set</c>
-          datatype, time is proportional to the (binary) logarithm of
-          the number of objects.</p>
+          in the resulting list, and so on. See also the note about
+          <seeerl marker="#insert_list_order">list insertion order</seeerl>.
+        </p>
       </desc>
     </func>
 

lib/stdlib/test/ets_SUITE.erl

@@ -51,7 +51,8 @@
 -export([t_insert_list/1, t_insert_list_bag/1, t_insert_list_duplicate_bag/1,
          t_insert_list_set/1, t_insert_list_delete_set/1,
          t_insert_list_parallel/1, t_insert_list_delete_parallel/1,
-         t_insert_list_kill_process/1]).
+         t_insert_list_kill_process/1,
+         t_insert_list_insert_order_preserved/1]).
 -export([test_table_size_concurrency/1,test_table_memory_concurrency/1,
          test_delete_table_while_size_snapshot/1, test_delete_table_while_size_snapshot_helper/1,
          test_decentralized_counters_setting/1]).
@@ -222,6 +223,7 @@ groups() ->
        t_insert_list_duplicate_bag, t_insert_list_delete_set,
        t_insert_list_parallel, t_insert_list_delete_parallel,
        t_insert_list_kill_process,
+       t_insert_list_insert_order_preserved,
        insert_trap_delete,
        insert_trap_rename]}].
 
@@ -1553,6 +1555,32 @@ t_insert_list_kill_process_do(Opts) ->
                                        fun ets:insert_new/2]],
     ok.
 
+t_insert_list_insert_order_preserved(Config) when is_list(Config) ->
+    insert_list_insert_order_preserved(bag),
+    insert_list_insert_order_preserved(duplicate_bag),
+    ok.
+
+insert_list_insert_order_preserved(Type) ->
+    Tab = ets:new(?FUNCTION_NAME, [Type]),
+    K = a,
+    Values1 = [{K, 1}, {K, 2}, {K, 3}],
+    Values2 = [{K, 4}, {K, 5}, {K, 6}],
+    ets:insert(Tab, Values1),
+    ets:insert(Tab, Values2),
+    [{K, 1}, {K, 2}, {K, 3}, {K, 4}, {K, 5}, {K, 6}] = ets:lookup(Tab, K),
+
+    ets:delete(Tab, K),
+    [] = ets:lookup(Tab, K),
+
+    %% Insert order in duplicate_bag depended on reductions left
+    ITERATIONS_PER_RED = 8,
+    NTuples = 4000 * ITERATIONS_PER_RED + 10,
+    LongList = [{K, V} || V <- lists:seq(1, NTuples)],
+    ets:insert(Tab, LongList),
+    LongList = ets:lookup(Tab, K),
+
+    ets:delete(Tab).
+
 %% Test interface of ets:test_ms/2.
 t_test_ms(Config) when is_list(Config) ->
     EtsMem = etsmem(),