Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dA8Lo-0003ZR-NJ for pgadmin-hackers@arkaria.postgresql.org; Mon, 15 May 2017 05:15:21 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dA8Lo-0008OR-5Y for pgadmin-hackers@arkaria.postgresql.org; Mon, 15 May 2017 05:15:20 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dA8LY-0007vN-EC for pgadmin-hackers@postgresql.org; Mon, 15 May 2017 05:15:04 +0000 Received: from mail-pf0-x22f.google.com ([2607:f8b0:400e:c00::22f]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dA8LU-0004h1-FO for pgadmin-hackers@postgresql.org; Mon, 15 May 2017 05:15:03 +0000 Received: by mail-pf0-x22f.google.com with SMTP id 9so17053125pfj.1 for ; Sun, 14 May 2017 22:15:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=KoabUivBjIS0/ovp/0gLriaJR/QV8tXJbw5uz7hJtvU=; b=1jeYCNtS+IZe9HbBuyKwUPPhTU4TpCweyrJk444zTikaTZM4Gg1mNSpdMU05hsjnZT dvlqi1VDCzXHWeWT4RsC6RaWYeqrV6qHjVprPp95mYnFD3w2LDhBSZhGZ/bubEiX7+di BV3yN5vyM75HXJdSt/5uN1UPs5jVlC59wZCodLwS/CpjOMQd3Kwic8zkbARKc5qYg95X jf67zi3vrO768XuZt2ogBh9bw6A74Zm07lJgfCaIlT3VCYN1+lzy4lUAYxNwPsemX5SF fHPcnAyD1d2bsTMI5E7NMU3WJi5cQ5vTMXexet0GQHsQ39ktuf46qZONJ6FF/k+EqvXi A+kw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=KoabUivBjIS0/ovp/0gLriaJR/QV8tXJbw5uz7hJtvU=; b=HWY9B9tRBzJyQ5ywNLD1FKE/qBNtxrl2DusurCLaVHBKElxmZaqJmypUwBz+436qGL y8z4MuJS2xYjacChnHRId4zH5n7oOCT+K5Jc46QEDz4TgWHR9Z1S9xtBbBDAimUtKvXP qdboscDuVp87Nsi/CXER6aqrtqg9WLlCcJbtu59/k3spnAm78oB0wqggmQv5fnUQhWKG aiF5M4l58KFeoQ1nPtokaRmrTj/YfXR9i/5MglLpN9ZqbQEwx+XKA8a6/49ELcaIVhwy /zT7RO08fsjccfSe4OdkWtVlxIdyKF91DUnc8A6Ke131Qyiq/7iNuJGNo63h7u4VsmIn f+iw== X-Gm-Message-State: AODbwcAGkbp2/EbpzXOWVtmoFEUXFK51147mDzHB/C9YmAyUvbUv6gud 9YT5uMPc+cV4+UU8ISNIPStj6NdSW+GI X-Received: by 10.99.109.129 with SMTP id i123mr4227708pgc.103.1494825299487; Sun, 14 May 2017 22:14:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.100.170.75 with HTTP; Sun, 14 May 2017 22:14:58 -0700 (PDT) In-Reply-To: References: <19B13ECE-CA6A-48E7-BF70-264F490F40B4@pgadmin.org> From: Akshay Joshi Date: Mon, 15 May 2017 10:44:58 +0530 Message-ID: Subject: Re: [patch] Dependents and Dependencies in GreenPlum To: Matthew Kleiman Cc: Joao Pedro De Almeida Pereira , pgadmin-hackers , George Gelashvili Content-Type: multipart/alternative; boundary="f403045c5052fa4112054f89222a" X-Pg-Spam-Score: -1.9 (-) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --f403045c5052fa4112054f89222a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks patch applied. On Mon, May 15, 2017 at 6:19 AM, Matthew Kleiman 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.joshi@enterprisedb.com> wrote: > >> Hi Joao >> >> On Fri, May 12, 2017 at 11:23 PM, Joao Pedro De Almeida Pereira < >> jdealmeidapereira@pivotal.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 fi= rst >> 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 =3D self.server.copy() >> self.server_with_modified_user[*'username'*] =3D *"testpgadmin"* >> >> >> Modified setup function: >> >> >> def setUp(self): >> >> with test_utils.Database(self.server) as (connection, database_name)= : >> cursor =3D 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 =3D self.server.copy() >> self.server_with_modified_user['username'] =3D "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@pivotal.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 extr= a >>>> 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 t= able? >>>> >>>> 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.vashi@enterprisedb.com> wrote: >>>> >>>>> On Thu, May 11, 2017 at 12:03 PM, Dave Page wrote= : >>>>> >>>>>> >>>>>> >>>>>> On 11 May 2017, at 07:11, Akshay Joshi >>>>>> wrote: >>>>>> >>>>>> Hi Sarah >>>>>> >>>>>> On Thu, May 11, 2017 at 2:30 AM, Sarah McAlear >>>>>> 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 !=3D 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 expe= cting >>>>>>> when a table is created. To ensure that this role is created, we ha= d to >>>>>>> create a new user and use it to execute all the tests. After we mad= e 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=3D"test" in >>>>>> test_config.json file. But still I am unable to understand why it do= esn'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 itse= lf. >>>>> 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=C3=A3o & Sarah >>>>>>> >>>>>>> On Wed, May 10, 2017 at 1:56 PM, Akshay Joshi < >>>>>>> akshay.joshi@enterprisedb.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On May 10, 2017 19:07, "Sarah McAlear" 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=C3=A3o & Sarah >>>>>>>> >>>>>>>> On Wed, May 10, 2017 at 2:29 AM, Akshay Joshi < >>>>>>>> akshay.joshi@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi Sarah >>>>>>>>> >>>>>>>>> On Tue, May 9, 2017 at 9:03 PM, Sarah McAlear >>>>>>>> > 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 colu= mn. >>>>>>>>>>> - 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 tri= ggers. >>>>>>>>>> >>>>>>>>>> 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 !=3D= 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-824= 6 >> <+91%2097678%2088246>* >> > > --=20 *Akshay Joshi* *Principal Software Engineer * *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* --f403045c5052fa4112054f89222a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks patch applied.

=
On Mon, May 15, 2017 at 6:19 AM, Matthew Kleiman= <mkleiman@pivotal.io> wrote:
Hi Akshay

I really appreciate you= taking a look at this on a Sunday!=C2=A0
I am not able to get ac= cess to my work computer to run your changes directly. From what you are sa= ying, 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 c= ommit.

Thanks again for helping us get this into t= he release.=C2=A0

Best,
Matt
=

On Sun, May 14, 2017 at 4:52 AM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi Joao
=
On Fri, May 12, 2017 at 11:23 PM, Joao= Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io>= wrote:
Hello Akshay,

<= /div>
You can find attached a new version of the patch that changes the= test in question.=C2=A0
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

=C2=A0 =C2= =A0Test 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 w= e 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"=C2=A0and it works, we can create new role using database serv= er's password=C2=A0

Old setup function:
<= div>=C2=A0 =C2=A0 =C2=A0=C2=A0def setUp(self):=C2=A0 = =C2=A0 =C2=A0 =C2=A0=C2=A0

=C2=A0 =C2=A0 create_role(self= .server, "testpgadmin")
=C2=A0 =C2=A0 self.server_with= _modified_user =3D self.server= .copy()
=C2=A0 =C2=A0 self.server_with= _modified_user['username&#= 39;] =3D "testpgadm= in"


Modified setup function:


=C2=A0 =C2=A0def setUp(self):

    =
with test_utils.=
Database(self.server) as (connection, dat=
abase_name):
        cursor =3D connection.cursor()
        try:
            curso=
r.execute(
                "CREATE ROLE testpgadmin LOGIN PASSWORD '%s'"             =
   % self.server['db_password'])
        except <=
/span>Exception as exception:
            print(exception)
        connectio=
n.commit()

    self.ser=
ver_with_modified_user =3D self.server.copy()
    self.se=
rver_with_modified_user['username'] =3D "testpgadmin"

= =C2=A0 =C2=A0If 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@pi= votal.io> wrote:

Hello a= ll,
This test is trying to check if the sql to retrieve the role dependencies o= f 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 th= e 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 c= onnect 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 depend= ency 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.vashi@enterprisedb.com> wrote:
On Thu, May 11, 2017 at 12:03 PM, Dave Page <dpage@pgadmi= n.org> wrote:


On 11 May 201= 7, at 07:11, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
=
Hi Sarah
On Thu, May 11, 2017 at 2:30 AM,= Sarah McAlear <smcalear@pivotal.io> wrote:
Hi Ashkay!

TestTablesNodeSql hasn't failed but TestRoleDependenciesSql fa= iled with below error:
=C2=A0 =C2=A0 .....\test_role_dependenci= es_sql.py\", line 41, in assertio= ns\n=C2=A0self.assertEqual(1, len(fetc= h_result))\nAssertionError: 1 !=3D 0

We experienced a similar= problem when we started developing this patch. We noticed that we were con= necting 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 tha= t this role is created, we had to create a new user and use it to execute a= ll the tests. After we made this change, the tests passed.=C2=A0
=
Can you try to create a new user and use it to execute your = tests?

=C2=A0 =C2=A0 I did that= and it works. I have created new user 'test' with superuser privil= eges and update parameter db_username=3D"test" in test_config.jso= n file. But still I am unable to understand why it doesn't work with &#= 39;postgres' (default) user? =C2=A0

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 tabl= e object is trying to execute the role dependencies, so I think this test c= ase is not correct.
<= div>

Thanks,
Jo=C3=A3o & Sarah

On Wed, May 10, 2017 at 1:56 PM, Akshay Joshi <akshay.joshi@= enterprisedb.com> wrote:


<= span>On May 10, 2017 19:07, "Sarah McAlear" <smcalear@pivotal.io> wrote:<= br type=3D"attribution">
Hi Akshay!

Does this erro= r occur on 9.6 or 10.0? We tested it in 9.6 and all our tests pass.

=C2=A0 =C2=A0 On both 9.6 and 10.0. Query returned 0 rows whic= h is compared against 1 in assert statement.=C2=A0

Th= anks!
Jo=C3=A3o & Sarah

On Wed, May 10, 2017 at 2:29 AM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi Sarah

On Tue, May 9, 2017 at 9:03 PM, Sarah McAlear <sm= calear@pivotal.io> wrote:
Hi Akshay!
=C2=A0
Some test file names ended with "_sql_template.= py" do we need to add that string ?=C2=A0
we added this suffix after moving the tests up a level to= tables/tests to clarify what subject they applied to. we changed the suffi= x to "_sql.py"

- Files "= test_column_acl_sql_templa= te.py" and "test_column_p= roperties_sql_template.py" should be = moved from tables->tests to tables->column->tests. As it's rel= ated to column.
- Files "test_trigger_get_= oid_sql_template.py" an= d "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.= =C2=A0

= Following test cases are failing
Thank you, fixed, see new patch. Can you confirm that TestTablesNodeSql = doesn't fail in your environment?
=C2=A0 = =C2=A0
=C2=A0 =C2=A0 TestTablesNodeSql hasn't failed b= ut TestRoleDependenciesSql failed with below error:
=C2=A0 =C2=A0= .....\test_role_dependencies_sql.py\", line 41, in assertions\n = self.assertEqual(1, len(fetch_result))\nA= ssertionError: 1 !=3D 0


Thanks,
George= & Sarah




--
Akshay Joshi
Principal Software Engi= neer=C2=A0

=







--
=
Akshay Joshi
Principal Software Engineer=C2= =A0


=
Pho= ne: +91 20-3058-9517
Mobile: +91 976-788-8246<= /b>




--
Akshay Joshi


<= b>Phone: +91 20-3058-9517
Mobile: <= a href=3D"tel:+91%2097678%2088246" value=3D"+919767888246" target=3D"_blank= ">+91 976-788-8246




--
=
Akshay = Joshi
Principal Software Engineer=C2=A0
=
=
Phone: +91 20-3058-9517
Mobil= e: +91 976-788-8246
--f403045c5052fa4112054f89222a--