Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file.

[Dave Page]

Hi

On Thu, Jun 3, 2021 at 7:39 AM Nikhil Mohite 
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?

-- 
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: https://www.enterprisedb.com

Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file.

[Nikhil Mohite]

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.moh...@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.
(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.

Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file.

[Dave Page]

Hi

On Thu, Jun 3, 2021 at 10:01 AM Nikhil Mohite <
nikhil.moh...@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.moh...@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

Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file.

[Nikhil Mohite]

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.moh...@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.moh...@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

Re: [pgAdmin][RM-6460]: Need a mechanism to detect a corrupt/broken config DB file.

[Nikhil Mohite]

Hi Team,

PFA updated patch (v2)
On Thu, Jun 3, 2021 at 3:10 PM Nikhil Mohite 
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.moh...@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.moh...@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


RM_6460_v2.patch
Description: Binary data