Re: [patch] Dependents and Dependencies in GreenPlum

Thanks patch applied.

On Mon, May 15, 2017 at 6:19 AM, Matthew Kleiman <mkleiman(at)pivotal(dot)io>
wrote:

> 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 <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Joao
>>
>> On Fri, May 12, 2017 at 11:23 PM, Joao Pedro De Almeida Pereira <
>> jdealmeidapereira(at)pivotal(dot)io> 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 <
>>> jdealmeidapereira(at)pivotal(dot)io> 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 <
>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> On Thu, May 11, 2017 at 12:03 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 11 May 2017, at 07:11, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
>>>>>> wrote:
>>>>>>
>>>>>> Hi Sarah
>>>>>>
>>>>>> On Thu, May 11, 2017 at 2:30 AM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>>>>>> 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 <
>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On May 10, 2017 19:07, "Sarah McAlear" <smcalear(at)pivotal(dot)io> 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 <
>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>
>>>>>>>>> Hi Sarah
>>>>>>>>>
>>>>>>>>> On Tue, May 9, 2017 at 9:03 PM, Sarah McAlear <smcalear(at)pivotal(dot)io
>>>>>>>>> > 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>*
>>
>
>

--
*Akshay Joshi*
*Principal Software Engineer *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*