Follow Plugin Optimizations OBO

[mvandeberg]

The implementation for #1947 is incorrect.

template< performance_data::t_creation_type CreationType, typename Index >
uint32_t performance_impl::delete_old_objects( const Index& old_idx, const account_name_type& start_account, uint32_t max_size, performance_data& pd ) const
{
   auto it_l = old_idx.lower_bound( start_account );
   auto it_u = old_idx.upper_bound( start_account );

   if( it_l == it_u )
      return 0;

   uint32_t next_id = get_actual_id( *it_l ) + 1;

   auto r_end = old_idx.rend();
   decltype( r_end ) it( it_u );

   bool is_init = true;

   while( it != r_end && it->account == start_account && next_id - get_actual_id( *it ) > max_size )
   {
      auto old_itr = it;
      ++it;

      remember_last< CreationType >( pd.s.is_empty, is_init, old_itr, pd );
   }

   if( !is_init )
     modify< CreationType >( *std::prev( it ), start_account, next_id, pd );

   return next_id;
}

Just before the while loop it is implied that it_l and it_u are not equal and both point to entries in the account's feed. This is wrong. I wrote a simple test that if the iterators don't both point to the same account, then log and my console was spammed.

I added the following directly before the while loop.

if( it_u != old_idx.end() && it_l != old_idx.end() && ( it_u->account != start_account || it_l->account != start_account ) )
      idump( (start_account)(it_l->account)(it_u->account) );

This could be logged on the first post for each account, but repeat authors were showing up, indicating a persistent problem. We had a node running develop crash due to a uniqueness constraint violation. I believe it is probably on the ordered unique index ( account, blog_feed_id).

The explanation is simple.

Lower Bound: Returns an iterator pointing to the first element in the range [first,last) which does not compare less than val.

Upper Bound: Returns an iterator pointing to the first element in the range [first,last) which compares greater than val.

It should be clear that upper bound will either return rend() or the next account. There is already a check that the iterators are not equal (which is trivially true assuming the index is non-empty). A quick decrement of it_u points it back to the account at the other end of the feed.

I believe there is another problem when the account is first posting. it_u and it_l point to a feed that does not belong to the account. The while loop will not enter and next_id for an feed that does not belong to the account will be returned. This is not as problematic but it means that feed ids do not necessarily start at 0.

Due to the number of problems and apparent lack of testing, I am going to revert the merge of the original issue until these changes can be made and are properly tested. A simple test of user feeds and blogs against production should be sufficient to prove correctness.

[goldibex]

@mvandeberg how did this get merged to begin with?

[mvandeberg]

I did not review it as carefully as I should have and assumed it had been tested.

[mariusztrela]

@mvandeberg

1. The bug has been confirmed to be fixed and PR 2018 was created with the fixes.
   The fix was confirmed by running reindex until it synchronized with head block last friday. Earlier I confirmed the crash running reindex without the fix.

2. The bug slipped through the testing I did before the crash occurred, because the testing has been done up until 17milionth block. The number was not chosen arbitrary - that was a head block at the time we started testing of our optimizations, and was not increased later in order to easily compare performance of following changes.

3. As you can see in PR (or in specific commit comment), the bug consisted of missing check. I accidentally removed it when modifying original loop. The bug has nothing to do with the iterators that you focused on. This requires some explanation:
   While your comments regarding iterators it_u and it_l and lower_bound or upper_bound are correct, they do not apply to my code because it_u is not used in the loop. Instead reverse iterator it is used and it is used correctly according to c++ spec of how iterator is converted to reverse iterator:
   http://en.cppreference.com/w/cpp/iterator/reverse_iterator
   In my code it is done in decltype( r_end ) it( it_u ); line.

4. Regarding your other comment
   This is not as problematic but it means that feed ids do not necessarily start at 0. - If given account doesn't have any feed, then always starts from zero value. When any feed_object exists for given account, then we don't enter to condition with return 0; statement.