139632 - ConnectToDatabase misuse - partial fix for 2.16

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

We call ConnectToDatabse() in a whole lot of places. Instead, we should only do
so at the top of scripts. Currently, we have the problem noted on landfill where
if you try to enter a bug when you're not logged in, and we're scheduled rebuild
the versioncache, then we clear the token table first. Rebuilding the
versioncache calls ConnectToDB, and if you're logged in with a non-0 groupset we
call userInGroup, which also connects to the db.

The token code doesn't, and so we fail when making a db query to do the updates

This is thus a 2.16 blocker.

The easy solution is just to always connectToDatabase at the top of scripts, and
never anywhere else. This will cause a sperious connection on pages which don't
otherwise need the versioncache, when a person isn't logged in (wince then we
wouldn't need them for the footer, either). I think that the product selection
part of enter_bug.cgi is the only one of those we have, so I think we can live
with that...

This blocks the replication bug, bug 124589, because there I've changed
ConnectToDatabase() to mean 'connect to the real db' instead of the current
behaviour of returning if we already have a connection. Yes, I could make
ConnectoToDatabase with no args only connect if we're not connected, but I think
that its easier to just remove the calls from helper functions, which are
scattered thoguhtout the code, as seen from the current issue with enter_bug.

MattyT has longer-term plans for a transaction API. Those shouldn't affect this,
though.

Comment 1

[Bradley Baetz (:bbaetz)]

I'll hack on this later tonight, if I get time.

Comment 2

[Dave Miller [:justdave]]

Since it's possible to tell if there were any arguments at all passed, I think
it would be safer (and probably more useful in general to prevent spurious
connections) if we make ConnectToDatabase() return if we have a connection,
ConnectToDatabase(0) connect to the real database, and ConnectToDatabase(1)
connect to the shadow database.

Comment 3

[Matthew Tuck [:CodeMachine]]

This is probably against comment #2, but what if we take a slightly different
approach to what I wanted on IRC?

We make ConnectToDatabase (or some appropriately renamed sub) be called at the
top of every script, specifying which database to use, but it doesn't do the
actual connection, this is done by SendSQL when needed, to the database
specified by ConnectToDatabase.

This way we get laziness in database connection, and we avoid specifying the
database to SendSQL and avoiding the problems that would cause.

Comment 4

[Matthew Tuck [:CodeMachine]]

$::selecteddb = 0;

sub SelectDatabase ($) { ($::selecteddb) = @_; }

sub SendSQL ... {

    my $db;

    if ($::selectddb == 0) {
	if ($::maindbconn is not connected) {
           connect $::maindbconn;
        }
        $db = $::maindbconn;
    }
    else if ($::selecteddb == 1) {
        if ($::shadowdbconn is not connected) {
           connect $::shadowdbconn;
        }
        $db = $::shadowdbconn;
    }
    else {
        error ...
    }

    use the $db variable ...

    ...

}

   

Comment 5

[Myk Melez [:myk] [@mykmelez]]

So what's the fix for the token code?  If this is only a blocker because of that
problem, this bug should only be about fixing that problem.  Otherwise this
isn't a blocker.

Comment 6

[Matthew Tuck [:CodeMachine]]

This problem was initially discovered on query.cgi but could not be later
reproduced.  The causes of that could be totally different.  We also don't know
how many of these problems exist in the new templatised code.  We only know how
to fix them.

Comment 7

[Bradley Baetz (:bbaetz)]

What mattyt said.

However, an acceptable 2.16 fix would just be to make sure that
connectToDatabase is called from every cgi. We can handle cleaning up the mess
later.

enter_bug.cgi is the only major cgi which doesn't call it always, and I'd guess
that we'd trigger that code only 5 times per hour at most, which as I said above
should be ok, I feel.

quips.cgi is the same as enter_bug in that the db isn't used, but that overhead
is almost certainly none, since bmo doesn't allow people to enter quips anyway.
It doesn't use the versioncache though, either, so it won't try to be rebuilt.

xml.cgi also doesn't call connectToDatabase explicitly, but it has to connect to
the db.

doeditparams doesn't, but it calls confirm_login, which connects. Most of the
edit* stuff has the same thing, as does userprefs.cgi.

index.cgi doesn't need the db if you're not logged in, but it does connect.
Thats certainly more overhead than enter_bug.

Note that any page with a footer will connect to the database if you have a
cookie (via GetCommandMenu), so we're only discussing cases where people arne't
logged in at all - was mistaken in my previous comment, and 100 is probably a
high number.

So, for 2.16 I suggest adding ConnectToDatabase calls to those cgis which don't
call them currently. enter_bug.cgi is the only blocker for this, though.

We can remove the uneeded ones post-release.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes blocker problem

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 80840 [details] [diff] [review]
patch v1: fixes blocker problem

2xr=gerv.

Gerv

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Blocker resolved, reducing severity.

Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.65; previous revision: 1.64
done

Comment 11

[Gervase Markham [:gerv]]

Pushing out to 2.18. The major problem is solved.

Gerv

Comment 12

[Dave Miller [:justdave]]

I don't want this on the "ready to check in" list for 2.18 if it's not ready to
check in.

Let's open a new bug to finish this.

Comment 13

[Matthew Tuck [:CodeMachine]]

Rule 1: Don't resolve partial bugs as fixed unless you've already opened another
bug.