Re: [patch] Dependents and Dependencies in GreenPlum

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>*
>