Merge lp:~ajayaa/drizzle/trunk-bug-951788 into lp:drizzle

On Mon, 16 Apr 2012 21:37:20 -0000, Ajaya Agrawal <email address hidden> wrote:
> === modified file 'client/server_detect.cc'
> --- client/server_detect.cc 2012-02-06 22:25:01 +0000
> +++ client/server_detect.cc 2012-04-16 21:36:20 +0000
> @@ -1,61 +1,37 @@
> -/* -*- mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; -*-
> - * vim:expandtab:shiftwidth=2:tabstop=2:smarttab:
> - *
> - * Copyright (C) 2011 Andrew Hutchings
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; version 2 of the License, or
> - * (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> 02110-1301 US

You shouldn't (i.e. must not) remove the (C) header. You can add your
name, but certainly not remove it.

--
Stewart Smith

Hi Ajaya

Thanks for attacking this bug!

Can you please pick a specific variable, not just vc_%. For instance SHOW VARIABLES LIKE 'vc_branch'

Please explain why you didn't go with "SHOW VARIABLES LIKE 'vc_%'" like we discussed in IRC. (I think your choice here is probably ok, just want you to explain it for the record.)

Hi Henrik,

    I believe that in IRC we talked about executing SHOW VARIABLES LIKE
"character_set_results" which returns an empty set for drizzle and some
rows for MYSQL.Similarly in the query SHOW VARIABLES LIKE 'vc_%, It returns
an empty set incase of MYSQL and some rows in case of drizzle because
there is no system variable in MYSQL starting with such prefixes.We could
use the query that we discussed in the IRC too.

And what advantage is there in using the query SHOW VARIABLES LIKE
'vc_branch' over SHOW VARIABLES LIKE 'vc_%' ???

On Tue, Apr 17, 2012 at 8:37 PM, Henrik Ingo <email address hidden>wrote:

> Hi Ajaya
>
> Thanks for attacking this bug!
>
> Can you please pick a specific variable, not just vc_%. For instance SHOW
> VARIABLES LIKE 'vc_branch'
>
> Please explain why you didn't go with "SHOW VARIABLES LIKE 'vc_%'" like we
> discussed in IRC. (I think your choice here is probably ok, just want you
> to explain it for the record.)
> --
> https://code.launchpad.net/~aj--/drizzle/trunk-bug-951788/+merge/102187<https://code.launchpad.net/%7Eaj--/drizzle/trunk-bug-951788/+merge/102187>
> You are the owner of lp:~aj--/drizzle/trunk-bug-951788.
>

--
Warm Regards,
 Ajaya Kumar Agrawal

Sorry, yes I meant character_set_results :-)

Ok, I suppose using one of the vc_ variables is good too. At least they should be in drizzle core, so they are not dependant on any particular plugin loaded.

Selecting a particular variable is more robust than using a wildcard. Now, if MySQL ever adds a variable that starts wtih vc_, this fix will not work anymore. However, if you check specifically for (example) vc_release_id then the likelihood that MySQL will ever have that exact variable is much smaller. Please remove the wildcard, other than that I'm ok with this.

Hi Henrik,

            I have changed the wildcard (vc_%) to more robust
(vc_release-id) and it works.
            Thanx for your valuable inputs.

On Wed, Apr 18, 2012 at 10:34 AM, Henrik Ingo <email address hidden>wrote:

> Sorry, yes I meant character_set_results :-)
>
> Ok, I suppose using one of the vc_ variables is good too. At least they
> should be in drizzle core, so they are not dependant on any particular
> plugin loaded.
>
> Selecting a particular variable is more robust than using a wildcard. Now,
> if MySQL ever adds a variable that starts wtih vc_, this fix will not work
> anymore. However, if you check specifically for (example) vc_release_id
> then the likelihood that MySQL will ever have that exact variable is much
> smaller. Please remove the wildcard, other than that I'm ok with this.
> --
> https://code.launchpad.net/~aj--/drizzle/trunk-bug-951788/+merge/102187<https://code.launchpad.net/%7Eaj--/drizzle/trunk-bug-951788/+merge/102187>
> You are the owner of lp:~aj--/drizzle/trunk-bug-951788.
>

--
Warm Regards,
 Ajaya Kumar Agrawal

Aprroved but with two things to note:

1. I'm not 100% happy with allocating the result struct in code and then freeing using the API (in my opinion the API should do it all)
2. We should put in release notes that this will no longer work with Drizzle versions prior to 7.0 GA (since that variable was added in Feb 2011)

This is probably how I would have done it:

const char *safe_query = "SHOW VARIABLES LIKE 'vc_release_id'";
drizzle_result_st* result= NULL;
drizzle_return_t ret_ptr;
result= drizzle_query_str(connection, NULL, safe_query, &ret_ptr);

if(ret_ptr == DRIZZLE_RETURN_OK)
{
  ret_ptr = drizzle_result_buffer(result);
  if (drizzle_result_row_count(result) > 0)
    type = SERVER_DRIZZLE_FOUND;
  }
  else
  {
    type = SERVER_MYSQL_FOUND;
  }
}

...

drizzle_result_free(result);

Actually, switching to needs fixing as I'm pretty sure there will be a memory leak on the result struct

   You are correct Andrew! I think it is because in drizzle_result_free
function we have something like if result->-options.is_allocated is true
then delete result. And while allocating in memory for drizzle_result_st we
are setting
this variable to false if it is already allocated memory somewhere.

On Fri, Apr 20, 2012 at 10:58 PM, Andrew Hutchings
<email address hidden>wrote:

> Review: Needs Fixing
>
> Actually, switching to needs fixing as I'm pretty sure there will be a
> memory leak on the result struct
> --
> https://code.launchpad.net/~aj--/drizzle/trunk-bug-951788/+merge/102187<https://code.launchpad.net/%7Eaj--/drizzle/trunk-bug-951788/+merge/102187>
> You are the owner of lp:~aj--/drizzle/trunk-bug-951788.
>

--
Warm Regards,
 Ajaya Kumar Agrawal

Applied the suggested changes!

On Fri, Apr 20, 2012 at 11:57 PM, Brian Aker <email address hidden> wrote:

> The proposal to merge lp:~aj--/drizzle/trunk-bug-951788 into lp:drizzle
> has been updated.
>
> Status: Approved => Needs review
>
> For more details, see:
> https://code.launchpad.net/~aj--/drizzle/trunk-bug-951788/+merge/102187<https://code.launchpad.net/%7Eaj--/drizzle/trunk-bug-951788/+merge/102187>
> --
> https://code.launchpad.net/~aj--/drizzle/trunk-bug-951788/+merge/102187<https://code.launchpad.net/%7Eaj--/drizzle/trunk-bug-951788/+merge/102187>
> You are the owner of lp:~aj--/drizzle/trunk-bug-951788.
>

--
Warm Regards,
 Ajaya Kumar Agrawal

The reason for this is there are currently two patterns for creating objects in libdrizzle. Either allocating and deleting in the app or getting the library to do it for you. Unfortunately it is too easy to mix the two.

Anyway, thanks for fixing this.

You need to merge first:

https://jenkins.drizzle.org/view/Build/job/start-build/41/console