Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lojVY-0002Pn-Mu for pgadmin-hackers@arkaria.postgresql.org; Thu, 03 Jun 2021 09:19:20 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lojVX-00064I-L7 for pgadmin-hackers@arkaria.postgresql.org; Thu, 03 Jun 2021 09:19:19 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lojVX-000649-5s for pgadmin-hackers@lists.postgresql.org; Thu, 03 Jun 2021 09:19:19 +0000 Received: from mail-ej1-x636.google.com ([2a00:1450:4864:20::636]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lojVT-0001aE-D0 for pgadmin-hackers@postgresql.org; Thu, 03 Jun 2021 09:19:17 +0000 Received: by mail-ej1-x636.google.com with SMTP id g20so8272791ejt.0 for ; Thu, 03 Jun 2021 02:19:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=iHoml1sHOjK+XJ6u5agfy2n8Zz3TgHNkEL/zl2OWn7k=; b=WbJ/aJIa1XQ5S32ZQDcGhKRCEm1C/psmLa4uQ1TSc/Or8gOZDdUZzmT2b5x11HDotN igSEp0JnA+pZ/7sOs20l5Qeqw76kHSkLUa/Nhte8VOBk5+2Pm+VFR9GbV6KF/RxaZMeQ VyWIA3UhNS6pfO/kbffqmqDJdsw6mjt/ZLZ2o2HJ2WWZP/M97SZ0kgNlT/F8q5Dit26Q 1e9iYO1PeiNrJxTpWft1QkJ4NYu2NN5dTkT6+z1hhkZNhPErljO/PFDK3XEQkHTic8Ep D0Hn1PXgdK6PPyER+95nFNOF5Sf46Kf/HuJzNr19+UI8l8JToa76zHdGURHzP2cASZer wPHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=iHoml1sHOjK+XJ6u5agfy2n8Zz3TgHNkEL/zl2OWn7k=; b=OoK2kb6d4cddFGaJZ+MkXG6CQ+GScRTQEZ7rEED4kpMXoCjQAkwXl6cTJN1NEP3QlQ IHjClB+5RrLxjW/2/h3vI9KfwQjCgU4VDOmYlAcqBfSY6GzDES0qv+ZvDcCN9Abi5dtM zisQ/UVRN/zkac8NRZoB5+PFuqz7/LA50wr7z9P4MuuYiUG7+lv05j2C7aWqx4NmYqUN BaneLvwsJzioSOkSpGu4qQ9yHzdo9SJ6tQGjsmbEVG/UOwNwcETOCUV+pl37Q1/TN5I/ Xwj2/s50+Y4K5zh9uSXeBt/3gRvOV0D/6GovVSjtjaecTgytBjWEql5Ff+8VvQM/3TFV UW2w== X-Gm-Message-State: AOAM533go6tgRhp2MhdqysgWq1OdqqJjSbRmOpQrA525HcSIeLXtUhSW BFRHHbJDkFrJ7kSwGzY2vl+89AxsYxpvHl57UVAI3g== X-Google-Smtp-Source: ABdhPJwI34q7nsCz1kU6urVFXllih1zU6iVKpn+fDW0FD56YA8zmrTIw7uNGMk+UjFryFm3AXljlRu65WTpfdQrHrFE= X-Received: by 2002:a17:906:161b:: with SMTP id m27mr38045513ejd.89.1622711953718; Thu, 03 Jun 2021 02:19:13 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Thu, 3 Jun 2021 10:19:00 +0100 Message-ID: Subject: Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file. To: Nikhil Mohite Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="00000000000093135a05c3d91192" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --00000000000093135a05c3d91192 Content-Type: text/plain; charset="UTF-8" Hi On Thu, Jun 3, 2021 at 10:01 AM Nikhil Mohite < nikhil.mohite@enterprisedb.com> wrote: > Hi Dave, > > On Thu, Jun 3, 2021 at 1:47 PM Dave Page wrote: > >> Hi >> >> On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohite < >> nikhil.mohite@enterprisedb.com> wrote: >> >>> Hi Hackers, >>> >>> Please find the attached patch for RM-6460 >>> : Need a mechanism to >>> detect a corrupt/broken config DB file. >>> >>> 1. Added checks if all tables added in the model are present in SQLite >>> DB or not. >>> 2. If migrations fail it will backup older file and try migrations with >>> the newly created file. >>> (User will get notification on UI for the location of the backup file >>> and newly created.) >>> 3. If the user deleted any table from SQLite DB pgAdmin will not run on >>> the next restart and it will add the missing table list in the logs. >>> >> >> Surely if any tables have been deleted, it'll fail the check in point 1? >> > Yes, but if the user deletes any table while pgAdmin is running then it > will fail when the user tries to run pgAdmin next time. > Right - but how is the end result of that different from a failed migration that left a table missing? Either way, the end result is the same, and should be handled the same way; i.e. backup/recreate the DB, then warn the user as soon as the UI is up. I believe the process is simple (and maybe this is what is done and this is just a language issue - I haven't read the patch in detail): - On startup, attempt to create the database/run any migrations as appropriate. - Check that all tables exist, and the schema version is correct. - If there's a problem, backup the file, create a new one from scratch, and warn the user. One thing I did notice when skimming the patch - we have this in the code: + raise RuntimeError( + 'Specified SQLite database file is not valid.') The *user* didn't specify a SQLite database file (nor do they care that it's SQLite at this point). That (and any other similar messages) should probably be rephrased to say something like: "The configuration database file is not valid." Thanks! > (If we remove the table from the model it will not check particular table > is present in DB or not. ) > >> >> -- >> Dave Page >> Blog: https://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EDB: https://www.enterprisedb.com >> >> Regards, > Nikhil Mohite. > -- Dave Page Blog: https://pgsnake.blogspot.com Twitter: @pgsnake EDB: https://www.enterprisedb.com --00000000000093135a05c3d91192 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Thu, Jun 3, 2021 at 10:01 AM Ni= khil Mohite <nikhil.mo= hite@enterprisedb.com> wrote:
Hi Dave,

On Thu, Jun 3, 2021 at 1:47 PM Dave Page <= dpage@pgadmin.org> wrote:

= On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:
Hi Hackers,
Please find the attached patch for=C2=A0RM-6460:=C2=A0 Need a= mechanism to detect a corrupt/broken config DB file.

<= div>1. Added checks if all tables added in the model are present in SQLite = DB or not.
2. If migrations fail it will backup older file and tr= y migrations with the newly created file.=C2=A0
(User will get no= tification on UI=C2=A0for the location=C2=A0of the backup file and newly cr= eated.)
3. If the user deleted any table from SQLite DB pgAdmin w= ill not run on the next restart and it will add the missing table list in t= he logs.

Surely if any tables h= ave been deleted, it'll fail the check in point 1?
Yes, but if the user deletes any table while pgAdmin is runn= ing then it will fail when the user tries to run pgAdmin next time.

Right - but how is the end resul= t of that different from a failed migration that=C2=A0left a table missing?= Either way, the end result is the same, and should be handled the same way= ; i.e. backup/recreate the DB, then warn the user as soon as the UI is up.<= /div>

I believe the process is simple (and maybe this is= what is done and this is just a language issue - I haven't read the pa= tch in detail):

- On startup, attempt to create th= e database/run any migrations as appropriate.
- Check that all ta= bles exist, and the schema version is correct.
- If there's a= problem, backup the file, create a new one from scratch, and warn the user= .

One thing I did notice when skimming the patch -= we have this in the code:

+ =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0raise RuntimeError(
+ = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0'S= pecified SQLite database file is not valid.')

The *user* didn't specify a SQLite database file (nor do they car= e that it's SQLite at this point). That (and any other similar messages= ) should probably be rephrased to say something like:

<= div>"The configuration database file is not valid."
Thanks!

=C2=A0
=
(If we remove the table fr= om the model it will not check particular table is present in DB or not. )<= /div>
=C2= =A0
--
Dave Page
Blog: = https://pgsnake.= blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

<= /div>
Regards,
Nikhil Mohite.=C2=A0<= /div>


--
<= /div>
--00000000000093135a05c3d91192--