Re: [PATCH v5 4/5] prserv: enable database sharing

[Michael Opdenacker <michael.opdenacker@bootlin.com>]

Hi Joshua

Many thanks for the review and guidance. See my comments and questions 
below.

On 4/29/24 at 22:34, Joshua Watt wrote:
>> +        with closing(self.conn.cursor()) as cursor:
>> +            if self.read_only:
>> +                table_exists = cursor.execute(
>> +                            "SELECT count(*) FROM sqlite_master \
>> +                            WHERE type='table' AND name='%s'" % (self.table))
>> +                if not table_exists:
>> +                    raise prserv.NotFoundError
> This is a race if you run the read-only server first, which seems like
> a bad user experience. I'd probably create the table even if the
> server is read-only (I'm pretty sure hash server works this way also)


Fixed. Thanks.

>
>> +            row = data.fetchone()
>> +            if row is not None:
>> +                val=row[0]
>> +            else:
>> +                #no value found, try to insert
>> +                try:
>> +                    cursor.execute("INSERT INTO %s VALUES (?, ?, ?, ?);"  % (self.table),
>> +                               (version, pkgarch, checksum, value))
>> +                except sqlite3.IntegrityError as exc:
>> +                    logger.error(str(exc))
> You probably want to make this whole SELECT / INSERT block a loop, and
> instead of repeating the SELECT below, retry the loop on an integrity
> error. The only reason you should be getting an integrity error is if
> you have a race between multiple servers accessing the database at
> once. In that case, looping back and trying to fall out in the
> previous SELECT is better.
>
> This is especially important if you delete entries in the database, as
> in pathological cases you might have to loop several times before you
> fall out of the loop (although typically, you would only need to retry
> once).
>
> I'd recommend this paradigm any other places you do SELECT / INSERT
> where the INSERT depends on the SELECT.

This is actually coming from the prserv legacy code, which I haven't 
really touched yet.

Is this kind of event really expected to happen with sqlite3? I see that 
hashserve only checks for "sqlite3.IntegrityError" once, so maybe it's 
too pessimistic to anticipate errors here... Do we also want to support 
multiple builders accessing the same PR database?

Anyway, if I really have to do it, I guess I'd also have to do it in the 
store_value() function which currently just logs an error without 
retrying. But if we loop, doesn't it add the risk to create an endless 
loop?

Cheers,
Michael.

-- 

Michael Opdenacker, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com