moodlehq / moodle-performance-comparison

Set of shell scripts to run Moodle performance tests using different hardware and configurations and compare results.
GNU General Public License v2.0
75 stars 39 forks source link

Add option to drop tables instead of schema drop/recreate #63

Closed mwithheld closed 8 months ago

mwithheld commented 4 years ago

PostGres code only

stronk7 commented 4 years ago

Ho @mwithheld ,

thanks for your idea and code! So all we want is to avoid dropping an recreating the DB all the time and, instead, just drop and recreate all the tables?

1) Can I ask which is the benefit for this? User lacking permissions or what? I imagine that dropping the DB can be really faster than dropping all individual objects one by one. Any comment about the use case?

2) We cannot accept half-baked solutions, only supporting postgres. The solution should offer cross-db support.

3) If you are going to go ahead with this... I'd also recommend you to re-indent all the code around your new conditions to follow the standard indentation rules.

Again, thanks! I think the main point is to ensure that there is a valid use case for the change. The other 2 points are just formal / style implementation details.

Ciao :-)

mwithheld commented 4 years ago

We typically work on a least-privileges basis, so my user was not owner or superuser, so could not drop the schema, but could drop and add tables. I went with the easier fix of making my user owner.

On Tue, Dec 10, 2019 at 3:14 PM Eloy Lafuente notifications@github.com wrote:

Ho @mwithheld https://github.com/mwithheld ,

thanks for your idea and code! So all we want is to avoid dropping an recreating the DB all the time and, instead, just drop and recreate all the tables?

1.

Can I ask which is the benefit for this? User lacking permissions or what? I imagine that dropping the DB can be really faster than dropping all individual objects one by one. Any comment about the use case? 2.

We cannot accept half-baked solutions, only supporting postgres. The solution should offer cross-db support. 3.

If you are going to go ahead with this... I'd also recommend you to re-indent all the code around your new conditions to follow the standard indentation rules.

Again, thanks! I think the main point is to ensure that there is a valid use case for the change. The other 2 points are just formal / style implementation details.

Ciao :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/moodlehq/moodle-performance-comparison/pull/63?email_source=notifications&email_token=AAGCSOXL5MIPEHIOAHWYU3TQYAPGFA5CNFSM4JP47462YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGRJHVI#issuecomment-564302805, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGCSOVE7ZUTFB2A52GUAK3QYAPGFANCNFSM4JP4746Q .

mwithheld commented 4 years ago

This would also allow you to use the same schema with a different prefix -- e.g. for those who can't get a second database approved.

On Tue, Dec 10, 2019 at 3:53 PM Mark v vhmark@gmail.com wrote:

We typically work on a least-privileges basis, so my user was not owner or superuser, so could not drop the schema, but could drop and add tables. I went with the easier fix of making my user owner.

On Tue, Dec 10, 2019 at 3:14 PM Eloy Lafuente notifications@github.com wrote:

Ho @mwithheld https://github.com/mwithheld ,

thanks for your idea and code! So all we want is to avoid dropping an recreating the DB all the time and, instead, just drop and recreate all the tables?

1.

Can I ask which is the benefit for this? User lacking permissions or what? I imagine that dropping the DB can be really faster than dropping all individual objects one by one. Any comment about the use case? 2.

We cannot accept half-baked solutions, only supporting postgres. The solution should offer cross-db support. 3.

If you are going to go ahead with this... I'd also recommend you to re-indent all the code around your new conditions to follow the standard indentation rules.

Again, thanks! I think the main point is to ensure that there is a valid use case for the change. The other 2 points are just formal / style implementation details.

Ciao :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/moodlehq/moodle-performance-comparison/pull/63?email_source=notifications&email_token=AAGCSOXL5MIPEHIOAHWYU3TQYAPGFA5CNFSM4JP47462YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGRJHVI#issuecomment-564302805, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGCSOVE7ZUTFB2A52GUAK3QYAPGFANCNFSM4JP4746Q .