public inbox for [email protected]
help / color / mirror / Atom feedFrom: Matthew Kleiman <[email protected]>
To: Akshay Joshi <[email protected]>
Cc: Joao Pedro De Almeida Pereira <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: George Gelashvili <[email protected]>
Subject: Re: [patch] Dependents and Dependencies in GreenPlum
Date: Sun, 14 May 2017 20:49:04 -0400
Message-ID: <CAFS4TJYdqp7z_gF8QB83a+nmC__+Xic_c=diK42MHh1oq-X1FA@mail.gmail.com> (raw)
In-Reply-To: <CANxoLDe8gxv0tYCq6kYt3LNbtZ8k5iEma+rTURtT0PF6QjksHg@mail.gmail.com>
References: <CAE+jjam5JoMt9+Ue-O_qpgEJO_wAGJfGoaLCJg-1VzVc+AqB_w@mail.gmail.com>
<CA+OCxoxOaYKi6j5dwgwEP7VtP-8XDiee101kjnO3e6SJc4Mp1g@mail.gmail.com>
<CAG7mmoyg2nbVHtYJVd1RA4e2orMv6nsaPoSbUeupy9akrz-z_w@mail.gmail.com>
<CAE+jja=8s-x82BVuBD4THq5V=07F_rJn0+Sa8vUGc=rOtxwWLA@mail.gmail.com>
<CAE+jja=dpFAAxavzJupBjcKQNZjox+grCiqd53AC2EO2G-FGdg@mail.gmail.com>
<CAGRPzo-pSDrQwXOHcy1aPbe==nNqRQpJeK6h2qr8_rsGiidRHg@mail.gmail.com>
<CAAtBm9V77+cbV5fbOkwrPg6JNDuVxQq5=ug7vdChAuTDY0Wj8Q@mail.gmail.com>
<CA+OCxozvcTmYOBrdXMi=b3w4f3LaBSYUMyJiTaUhz-uLuOAWiA@mail.gmail.com>
<CA+OCxozwGCNHJ1d58Huoc=zWGkvH3yWhrzRE9kE7oKm-LdQdYQ@mail.gmail.com>
<CANxoLDfcwCnw8ZRaVzT8TO4be1tmT1XSfOUrD=zMiud-cHLybA@mail.gmail.com>
<CANxoLDfGtTEyfwb=2snvRYN5qAr_CiC5s4J-_NMeSY8hUk8M7A@mail.gmail.com>
<CA+OCxoySRBh0YRRxOo-Osn3WOViPkcO-2Qt55iBjByzEb=bhtQ@mail.gmail.com>
<CAGRPzo9rkT+ehf-ShMvzU_TGyXT5eCek=jQDeSk5pnaMoQgzuQ@mail.gmail.com>
<CANxoLDc-j1rVx4Ke+Qjxpi82Nj4FfbhiYhriVY3ekNbtKBF2qQ@mail.gmail.com>
<CAGRPzo_ATD7Erwz=5Eay1GDfXnDq4DvEinYvfsSGQbVBmcxrNg@mail.gmail.com>
<CANxoLDfeJOiFKpsNa6aWU0Kui_3Pz6nswW=8uUE==8FUYJ3WoQ@mail.gmail.com>
<CAGRPzo95Yy4u-CQHFXXX=49Thci3mgdMvm3hxuJKNc0MhZU8tg@mail.gmail.com>
<CANxoLDcvq+eG482KkLzUY7sERM-y=KUeRUnuvskL600AvOkFdQ@mail.gmail.com>
<CAGRPzo8b8zfo2rX-ecL+8qdnS-2c1UJqv2nRzed-aTei_Pr5CA@mail.gmail.com>
<CANxoLDcS0+HMZx--k+xAwtRA0y9hqfMNvtgJgRxyWvFf3S7FqA@mail.gmail.com>
<[email protected]>
<CAFOhELcsS0KetQjDoWXeHiaFmCQmAEMJykqocaHG4n80NHqjgg@mail.gmail.com>
<CAE+jjamEUWDGk+7cx298-HXp-fuqgoXmTaAnn1Z7kL-P=MwwZQ@mail.gmail.com>
<CAE+jja=i5QXtYY3qj_m4Zv-APP3qnY4rrK_tv6VBFHusetWOUA@mail.gmail.com>
<CANxoLDe8gxv0tYCq6kYt3LNbtZ8k5iEma+rTURtT0PF6QjksHg@mail.gmail.com>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgadmin-hackers>
Hi Akshay
I really appreciate you taking a look at this on a Sunday!
I am not able to get access to my work computer to run your changes
directly. From what you are saying, though, it sounds like a reasonable
change. If this change is passing in your environment, I say go ahead and
make the change to the patch and commit.
Thanks again for helping us get this into the release.
Best,
Matt
On Sun, May 14, 2017 at 4:52 AM, Akshay Joshi <[email protected]
> wrote:
> Hi Joao
>
> On Fri, May 12, 2017 at 11:23 PM, Joao Pedro De Almeida Pereira <
> [email protected]> wrote:
>
>> Hello Akshay,
>>
>> You can find attached a new version of the patch that changes the test in
>> question.
>> This new version instead of requiring a creation of a new Role now does
>> the following:
>> 1 - Creates a new Role
>> 2 - Creates a table with that Role
>> 3 - Checks for dependencies
>>
>
> Test case still failing on my machine because when we create new Role
> it is without password and when we try to create table with that role first
> we will try to connect the database using that new role with
> servers['db_password'] which is wrong. I have modified setup function of "
> *test_role_dependencies_sql.py*" and it works, we can create new role
> using database server's password
>
> Old setup function:
> *def *setUp(self):
>
> create_role(self.server, *"testpgadmin"*)
> self.server_with_modified_user = self.server.copy()
> self.server_with_modified_user[*'username'*] = *"testpgadmin"*
>
>
> Modified setup function:
>
>
> def setUp(self):
>
> with test_utils.Database(self.server) as (connection, database_name):
> cursor = connection.cursor()
> try:
> cursor.execute(
> "CREATE ROLE testpgadmin LOGIN PASSWORD '%s'"
> % self.server['db_password'])
> except Exception as exception:
> print(exception)
> connection.commit()
>
> self.server_with_modified_user = self.server.copy()
> self.server_with_modified_user['username'] = "testpgadmin"
>
>
> If the above logic looks good to you then I'll commit the code.
>
>>
>> Thanks
>> Joao & George
>>
>> On Thu, May 11, 2017 at 7:33 AM, Joao Pedro De Almeida Pereira <
>> [email protected]> wrote:
>>
>>> Hello all,
>>> This test is trying to check if the sql to retrieve the role
>>> dependencies of a object in the database is correct. Using a different user
>>> to connect to the database and create tables there is no need for extra
>>> setup because the role depedencies table entry is added automatically. Also
>>> we talked with some internal users and they told us they always used a
>>> different user to connect to the database with pgadmin.
>>>
>>> After your comments we realize that the test need to have a different
>>> setup in order to become more readable and resilient. What do we need to do
>>> in the test setup to ensure an entry is added to the role dependency table?
>>>
>>> Also the test need to be a little bit more resilient to work
>>> independently of the user that is used on the test.
>>>
>>> Thanks
>>> Joao
>>>
>>> On Thu, May 11, 2017, 3:20 AM Khushboo Vashi <
>>> [email protected]> wrote:
>>>
>>>> On Thu, May 11, 2017 at 12:03 PM, Dave Page <[email protected]> wrote:
>>>>
>>>>>
>>>>>
>>>>> On 11 May 2017, at 07:11, Akshay Joshi <[email protected]>
>>>>> wrote:
>>>>>
>>>>> Hi Sarah
>>>>>
>>>>> On Thu, May 11, 2017 at 2:30 AM, Sarah McAlear <[email protected]>
>>>>> wrote:
>>>>>
>>>>>> Hi Ashkay!
>>>>>>
>>>>>> TestTablesNodeSql hasn't failed but TestRoleDependenciesSql failed
>>>>>>> with below error:
>>>>>>> .....\test_role_dependencies_sql.py\", line 41, in assertions\n
>>>>>>> self.assertEqual(1, len(fetch_result))\nAssertionError: 1 != 0
>>>>>>
>>>>>>
>>>>>> We experienced a similar problem when we started developing this
>>>>>> patch. We noticed that we were connecting to the database with the super
>>>>>> user (the db owner user), which did not create the role we are expecting
>>>>>> when a table is created. To ensure that this role is created, we had to
>>>>>> create a new user and use it to execute all the tests. After we made this
>>>>>> change, the tests passed.
>>>>>>
>>>>>> Can you try to create a new user and use it to execute your tests?
>>>>>>
>>>>>
>>>>> I did that and it works. I have created new user 'test' with
>>>>> superuser privileges and update parameter db_username="test" in
>>>>> test_config.json file. But still I am unable to understand why it doesn't
>>>>> work with 'postgres' (default) user?
>>>>>
>>>>>
>>>>> Agreed. That suggests something fishy is going on that should be
>>>>> understood.
>>>>>
>>>>>
>>>> We don't check role dependencies for every object. As per the code, the
>>>> role dependencies are executed for columns not for the table itself.
>>>> And in the test cases, the table object is trying to execute the role
>>>> dependencies, so I think this test case is not correct.
>>>>
>>>>>
>>>>>> Thanks,
>>>>>> João & Sarah
>>>>>>
>>>>>> On Wed, May 10, 2017 at 1:56 PM, Akshay Joshi <
>>>>>> [email protected]> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On May 10, 2017 19:07, "Sarah McAlear" <[email protected]> wrote:
>>>>>>>
>>>>>>> Hi Akshay!
>>>>>>>
>>>>>>> Does this error occur on 9.6 or 10.0? We tested it in 9.6 and all
>>>>>>> our tests pass.
>>>>>>>
>>>>>>>
>>>>>>> On both 9.6 and 10.0. Query returned 0 rows which is compared
>>>>>>> against 1 in assert statement.
>>>>>>>
>>>>>>>
>>>>>>> Thanks!
>>>>>>> João & Sarah
>>>>>>>
>>>>>>> On Wed, May 10, 2017 at 2:29 AM, Akshay Joshi <
>>>>>>> [email protected]> wrote:
>>>>>>>
>>>>>>>> Hi Sarah
>>>>>>>>
>>>>>>>> On Tue, May 9, 2017 at 9:03 PM, Sarah McAlear <[email protected]>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi Akshay!
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Some test file names ended with "*_sql_template.py*" do we need
>>>>>>>>>> to add that string ?
>>>>>>>>>
>>>>>>>>> we added this suffix after moving the tests up a level to
>>>>>>>>> tables/tests to clarify what subject they applied to. we changed the suffix
>>>>>>>>> to "_sql.py"
>>>>>>>>>
>>>>>>>>> - Files "test_column_acl_sql_template.py" and "
>>>>>>>>>> test_column_properties_sql_template.py" should be moved from
>>>>>>>>>> tables->tests to tables->column->tests. As it's related to column.
>>>>>>>>>> - Files "test_trigger_get_oid_sql_template.py" and "
>>>>>>>>>> test_trigger_nodes_sql_template.py" should be moved from
>>>>>>>>>> tables->tests to tables->triggers->tests. As its related to triggers.
>>>>>>>>>
>>>>>>>>> these tests are related to the sql files in
>>>>>>>>> tables/templates/column, not tables/column, so moving them into
>>>>>>>>> tables/column would be more confusing.
>>>>>>>>>
>>>>>>>>> Following test cases are failing
>>>>>>>>>
>>>>>>>>> Thank you, fixed, see new patch. Can you confirm that
>>>>>>>>> TestTablesNodeSql doesn't fail in your environment?
>>>>>>>>>
>>>>>>>>
>>>>>>>> TestTablesNodeSql hasn't failed but TestRoleDependenciesSql
>>>>>>>> failed with below error:
>>>>>>>> .....\test_role_dependencies_sql.py\", line 41, in assertions\n
>>>>>>>> self.assertEqual(1, len(fetch_result))\nAssertionError: 1 != 0
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> George & Sarah
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> *Akshay Joshi*
>>>>>>>> *Principal Software Engineer *
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> *Akshay Joshi*
>>>>> *Principal Software Engineer *
>>>>>
>>>>>
>>>>>
>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>
>>>>>
>>
>
>
> --
> *Akshay Joshi*
> *Principal Software Engineer *
>
>
>
> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 976-788-8246
> <+91%2097678%2088246>*
>
view thread (27+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: [patch] Dependents and Dependencies in GreenPlum
In-Reply-To: <CAFS4TJYdqp7z_gF8QB83a+nmC__+Xic_c=diK42MHh1oq-X1FA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox