Re: bzr commit into mysql-6.0-backup branch (oystein.grovlen:2699) Bug#39089 WL#4384

[Rafal Somla]

Hi Oystein,

I noticed that in your new patch you introduced changes like this one:

> @@ -1511,27 +1533,23 @@ void* bcat_iterator_get(st_bstream_image
>    case BSTREAM_IT_TABLESPACE:     // table spaces
>    {
>      Iterator *it= info->get_tablespaces();
> -  
> -    if (!it)
> +    if (!it) 
>      {
> -      info->m_ctx.fatal_error(ER_BACKUP_CAT_ENUM);
> -      return NULL;
> +      info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
>      }
> -
> +  
>      return it;
>    }

So, after your patch the code will look like this:

   case BSTREAM_IT_TABLESPACE:     // table spaces
   {
     Iterator *it= info->get_tablespaces();
     if (!it)
     {
       info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
     }

     return it;
   }


Note that this code assumes that if info->get_tablespaces() fails, then it 
failed because of missing resources. Whether this assumption is true or not, 
depends on the current implementation of Image_info::get_tablespaces() method.

While I can see the assumption is currently true, I think that such code is 
difficult to maintain. It creates a dependency: whenever someone changes 
implementation of get_tablespaces(), he should check if all implicit assumptions 
made elsewhere in the code are still valid and update the code accordingly if 
this is not the case. This is very unlikely to happen. First, how should the 
author of new implementation find all these places? Will he understand all 
assumptions which are not explicitly stated? Will he make no mistakes when 
updating code which is not his primary concern?

I think writing code whose correctness depends on implementation details of 
functions used in this code is a bad practice. Instead, we should consider 
function specifications as the binding contract of how one can use given 
function, not their implementations.

In this case, the specification of Image_info::get_tablespaces() should say how 
this function reports errors. The specification is not precise, unfortunately, 
but I think we agree that intention is that in case of errors function returns 
NULL. Since function's output is binary: either we get pointer to the iterator 
(in case of success) or NULL (in case of error), there is no way to know what 
caused the error.

So, if get_tablespaces() returns NULL we know that there was a problem, but we 
don't know its cause. And this was reflected in the original code which reported 
ER_BACKUP_CAT_ENUM, informing the user that it was not possible to enumerate 
backup catalogue. Since we don't know the exact reason of the problem, we don't 
report it.

Now, if we decide that this information is not enough, then I think the way to 
go is not to give more information based on the knowledge of internal 
implementation of get_tablespaces(). Instead, we should extend the interface of 
get_tablespaces() function so that it gives the caller more info about the cause 
of the problem.

What do you think?

Rafal