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 1lqcht-0004Vz-0W for pgadmin-hackers@arkaria.postgresql.org; Tue, 08 Jun 2021 14:27:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lqchr-0001c2-Vx for pgadmin-hackers@arkaria.postgresql.org; Tue, 08 Jun 2021 14:27:51 +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 1lqchr-0001bu-I8 for pgadmin-hackers@lists.postgresql.org; Tue, 08 Jun 2021 14:27:51 +0000 Received: from mail-il1-x12d.google.com ([2607:f8b0:4864:20::12d]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lqchp-0001Sn-3q for pgadmin-hackers@postgresql.org; Tue, 08 Jun 2021 14:27:50 +0000 Received: by mail-il1-x12d.google.com with SMTP id b5so19667375ilc.12 for ; Tue, 08 Jun 2021 07:27:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=V9RPFx/EmvbHigpy3J2GX8XjN3s2wzm0vcyrPTRHjPA=; b=Z6NLuo0N1tyJc8xSXvh+l/56jYyQbs79M+svcNuSEASk2RW8jtWOldwMbjAz6UGum8 ROe64Q+HK4U6n/d4jPbsJaB90hvXxHpANM9XOVcU3tXuajCe1fVLEFYQ0jGoltj9rSZU hQ3tRx4uKdnCoBa4U8bDnPhpKQQXOkVmSXE2M3fLbB8xchegtwCS38Xxg6qLiKhjD8qI xlLWbt8jAGhhI/sNsJTQLSzmEE/4LTSYoBOwUm1fzQthe0pSmlLt67ibyWgBT8+4we26 vuE2oiifaBJT9SOZyCcHeM/b7yzjQ7mxdlN4zdJlWJS++7yeU8m7yd2OPW42STECzFdC qdCg== 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=V9RPFx/EmvbHigpy3J2GX8XjN3s2wzm0vcyrPTRHjPA=; b=skjiZhNoLVrvaU2lk5Y4jJ4fDSkQCwUGvNdGveR4RpDyZ+QOipY80kqlsXmR3wTLq7 9oHXh5LOFHySBvCrxzL7LzMjMvy+q8PRlfzsA0ISRoTk62xeQsijPTvIVuZzojGd3BIz YOGr+graawRkULM9WD4uZT7AJSD0vqo/ejj3x8wybdiaglMhAbHyX+iICXfgC+MNR/01 2XtNS5gsGIlPULq4Mrx15HH/4uynSxNohDmsua7IuP4Z3uv8OxvJX458kIHcqJ0+yXhg O9gER5xdy5Q8bR2ySI+GTGCLfMXAMNGjL0uoOO7n4+Edn34kJkqwcohtgROUoS1MwGaI LRqg== X-Gm-Message-State: AOAM530ueGXN2sJWo896d++m9LJZmRO4wZy+YhtSLE8a7BUIHdmuxzvp QdHI/QrmwLCjMWRVZnU6MsGQF5sq+CSRNWajQXfKGQY02L32PgoGnC/eaDbJOQJ/b7nMAsDiXb7 YQbbSj2fmMUgwEtyZiFsfOq+O4RWAOp/qYXvIrDBQr1YZrh6/8JWgwJlEAnrBKFwHUEvnDc+8jO u9uXdnhUwbiHEnccoZoZyboL+AoU71CWT9PFTKg4REqIfHIQTz4JPYYa6mhA== X-Google-Smtp-Source: ABdhPJyFfqsNXm524Aq+nT0x5M4xqL9OrB8LHAiBPAy3fm7JNyToeHQw97ZStA9dTEejK7AC+0BfektwUdY8c5vm6PM= X-Received: by 2002:a05:6e02:4d0:: with SMTP id f16mr10932727ils.252.1623162468261; Tue, 08 Jun 2021 07:27:48 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Tue, 8 Jun 2021 19:57:37 +0530 Message-ID: Subject: Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file. To: Nikhil Mohite Cc: pgadmin-hackers , Dave Page Content-Type: multipart/alternative; boundary="0000000000005581cf05c441f646" X-CLOUD-SEC-AV-Info: enterprisedb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000005581cf05c441f646 Content-Type: text/plain; charset="UTF-8" Thanks, the patch applied. On Thu, Jun 3, 2021 at 3:52 PM Nikhil Mohite wrote: > Hi Team, > > PFA updated patch (v2) > On Thu, Jun 3, 2021 at 3:10 PM Nikhil Mohite < > nikhil.mohite@enterprisedb.com> wrote: > >> Hi Dave, >> >> On Thu, Jun 3, 2021 at 2:49 PM Dave Page wrote: >> >>> 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. >>> >> Yes - It should be the same in both conditions, I will send an updated >> patch for this. >> > >>> >> >>> 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 >>> >>> Regards, >> Nikhil Mohite >> > Regards, > Nikhil Mohite > -- *Thanks & Regards* *Akshay Joshi* *pgAdmin Hacker | Principal Software Architect* *EDB Postgres * *Mobile: +91 976-788-8246* --0000000000005581cf05c441f646 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks, the patch applied.

On Thu, Jun 3, 2021 at 3:52 = PM Nikhil Mohite <nikh= il.mohite@enterprisedb.com> wrote:
Hi Team,
<= div class=3D"gmail_quote">

<= div class=3D"gmail_attr">PFA updated patch (v2)
On Thu, Jun 3, 2021 at 3:10 PM Nikhil Mohite <nikhil.mohite@ent= erprisedb.com> wrote:
Hi Dave,

On Thu, Jun 3, 2021 = at 2:49 PM Dave Page <dpage@pgadmin.org> wrote:
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 <= dpage@pgadmin.org> wrote:
Hi

On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohi= te <= nikhil.mohite@enterprisedb.com> wrote:
Hi Hackers,

Please find the attached patch for=C2=A0RM-6460:=C2=A0 Need a mechanis= m to detect a corrupt/broken config DB file.

1. Ad= ded 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 migrati= ons with the newly created file.=C2=A0
(User will get notificatio= n on UI=C2=A0for the location=C2=A0of the backup file and newly created.)
3. If the user deleted any table from SQLite DB pgAdmin will not r= un on the next restart and it will add the missing table list in the logs.<= /div>

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=C2=A0left a table missing? Either w= ay, the end result is the same, and should be handled the same way; i.e. ba= ckup/recreate the DB, then warn the user as soon as the UI is up.
Yes - It should be the same in both conditi= ons, I will send an updated patch for this.=C2=A0
<= div dir=3D"ltr">
=C2=A0
<= br>
I believe the process is simple (and maybe this is what is do= ne and this is just a language issue - I haven't read the patch in deta= il):

- 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, b= ackup the file, create a new one from scratch, and warn the user.

One thing I did notice when skimming the patch - we have th= is 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'Specified SQLit= e database file is not valid.')

The *use= r* 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 proba= bly be rephrased to say something like:

"The = configuration database file is not valid."=C2=A0

Thanks!

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

EDB: https://www.enterprise= db.com

Regards,
Ni= khil Mohite.=C2=A0


--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake<= br>
EDB: http= s://www.enterprisedb.com

Regards,
Nikhil Mohite=C2=A0
Regards,
Nikhil Mohite=C2=A0


--
Thanks & Regards
Akshay Joshi
pgAdmi= n Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246

--0000000000005581cf05c441f646--