Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dA4Ck-0008Lb-39 for pgadmin-hackers@arkaria.postgresql.org; Mon, 15 May 2017 00:49:42 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dA4Ci-0006Rd-HP for pgadmin-hackers@arkaria.postgresql.org; Mon, 15 May 2017 00:49:40 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dA4Cg-0006RR-Jd for pgadmin-hackers@postgresql.org; Mon, 15 May 2017 00:49:38 +0000 Received: from mail-it0-x233.google.com ([2607:f8b0:4001:c0b::233]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dA4CV-0007nM-Of for pgadmin-hackers@postgresql.org; Mon, 15 May 2017 00:49:37 +0000 Received: by mail-it0-x233.google.com with SMTP id o5so34779329ith.1 for ; Sun, 14 May 2017 17:49:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pivotal-io.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=ETyod0hT1tWe09AkB6SvQAykh3VTG0z5QDiy3yzcNu0=; b=Ql6zQh1HkkTQ0MQbFxjq0jv98OvWwzqb3XVbz0rqlxgQ2NOLt7LBmntZET+G1Z5uPO fmQj3VjSr73phDw6ZjvFS/3dDpnxANJhzCqfFdtNb7+8Z++Nfaz09CLElx66pFo3kn92 PVDj+L+oBAGPNGAnBpfn1/OpVA637fme2bh8JMMtrwYmDdRo/WzJbpQcHKhi8m4x/8uy 5+2Gr38uPgui5Zv9grIb3uO5Cgvf9MiNq+g7Do896RH+j4MNKg+4JoojQgst488XJ83M ZsAx/mSCmwZmBMitwZslzVCfOXmS8TnXWzuX8432ZQJ+QW8YgH/AWhOIrDCGbpElFKfG JPzg== 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=ETyod0hT1tWe09AkB6SvQAykh3VTG0z5QDiy3yzcNu0=; b=cXEMLGBu+X/ha59Txzc/b/sDGPh7eOs8ChhJ/a7Eiwu6mWsM6F6tv4P3hJmdZ7zYkN GR3YDXVZCQwQ+B4lyeDonGRZDn+BBOVlyh6Ili7FyS87+xWB5F40zUqjPe1L4T9YylPA HNyavtUKS7aMI7v6p0ipuV2T7AKgrZsAJX3Yct8IzEc09cDs93rZa0tuJzzfRRkjP3eA T83Ij6ti/CfZ+KS0ehzMKlFB2pY5mgFRZwZqIHrB4ciOVObr7WNSEKGxm3MtMrZFSKhc KMUaBGbTm6lVeg3Jrdxf9rFPm6h7RWmM4PZGokZFRyScizcM7qVstAuqnBHLZJDZq1g0 M4uA== X-Gm-Message-State: AODbwcBI8b5CnLYvjxLZLlZRdIbJTahCcwYpAkSw7dhk2aNrLVxhK9Dk svUeZQicgwOE7od4RudSQbsjIH+O5jrb X-Received: by 10.36.57.137 with SMTP id l131mr3308511ita.61.1494809364656; Sun, 14 May 2017 17:49:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.185.70 with HTTP; Sun, 14 May 2017 17:49:04 -0700 (PDT) In-Reply-To: References: <19B13ECE-CA6A-48E7-BF70-264F490F40B4@pgadmin.org> From: Matthew Kleiman Date: Sun, 14 May 2017 20:49:04 -0400 Message-ID: Subject: Re: [patch] Dependents and Dependencies in GreenPlum To: Akshay Joshi Cc: Joao Pedro De Almeida Pereira , pgadmin-hackers , George Gelashvili Content-Type: multipart/alternative; boundary="001a114a9b66302f77054f856d5b" X-Pg-Spam-Score: -2.6 (--) 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 --001a114a9b66302f77054f856d5b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 i= n >> 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 fir= st > 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 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 t= o do >>> in the test setup to ensure an entry is added to the role dependency ta= ble? >>> >>> 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 s= uper >>>>>> user (the db owner user), which did not create the role we are expec= ting >>>>>> 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=3D"test" in >>>>> test_config.json file. But still I am unable to understand why it doe= sn'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, th= e >>>> 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=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 colum= n. >>>>>>>>>> - 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 trig= gers. >>>>>>>>> >>>>>>>>> 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-8246 > <+91%2097678%2088246>* > --001a114a9b66302f77054f856d5b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Akshay

I really appreciate you takin= g a look at this on a Sunday!=C2=A0
I am not able to get access t= o 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 yo= ur environment, I say go ahead and make the change to the patch and commit.=

Thanks again for helping us get this into the rel= ease.=C2=A0

Best,
Matt

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

You can find attached a new ve= rsion 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 foll= owing:
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 c= reate 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 mo= dified setup function of " test_role_dependencies_sql.py"=C2=A0and it works, we can c= reate new role using database server's password=C2=A0

Old setup function:
=C2=A0 =C2=A0 =C2=A0=C2=A0def setUp(self):=C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0
<= p style=3D"margin:0px;font-size:12px;line-height:normal;font-family:menlo"> =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 <jdealmeidap= ereira@pivotal.io> wrote:

= Hello all,
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 2017, at 07:11, Akshay Jo= shi <= akshay.joshi@enterprisedb.com> wrote:

Hi Sarah

O= n Thu, May 11, 2017 at 2:30 AM, Sarah McAlear <smcalear@pivotal.io&g= t; wrote:
Hi Ashkay!

TestTablesNodeSql hasn't failed but TestRoleDependenc= iesSql failed with below error:
=C2=A0 =C2=A0 .....\test_role_d= ependencies_sql.py\", line 41, in= assertions\n=C2=A0self.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), w= hich did not create the role we are expecting when a table is created. To e= nsure 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.=C2=A0

Can you try to create a new user and use it to exec= ute your tests?

=C2=A0 =C2=A0 I= did that and it works. I have created new user 'test' with superus= er privileges and update parameter db_username=3D"test" in test_c= onfig.json file. But still I am unable to understand why it doesn't wor= k with '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 ob= ject. As per the code, the role dependencies are executed for columns not f= or the table itself.
And in the test cases, the table object is t= rying to execute the role dependencies, so I think this test case is not co= rrect.
<= div class=3D"gmail_quote">
<= div dir=3D"ltr">

Thanks,
Jo=C3=A3= o & Sarah

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


On May 10, 2017 19:07, "Sarah McAlear" <smcalear@pivotal.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.
=

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

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 <= span dir=3D"ltr"><smcalear@pivotal.io> wrote:
Hi Akshay!
=C2=A0
Some te= st file names ended with "_sql_template.py" do we need to add that string ?=C2= =A0
we added this suffix after moving the te= sts 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-&g= t;tests to tables->column->tests. As it's related to column.
<= /span>- 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/te= mplates/column, not tables/column, so moving them into tables/column would = be more confusing.=C2=A0

Following test cases are failing
Tha= nk you, fixed, see new patch. Can you confirm that TestTablesNodeSql doesn&= #39;t fail in your environment?
=C2=A0 =C2=A0<= /div>
=C2=A0 =C2=A0 TestTablesNodeSql hasn't failed but Test= RoleDependenciesSql failed with below error:
=C2=A0 =C2=A0 .....\= test_role_dependencies_sql.py\", line 41, in assertions\n self.a= ssertEqual(1, len(fetch_result))\nAssertion= Error: 1 !=3D 0


Thanks,
George & S= arah




--
Akshay Joshi
= Principal Software Engineer=C2=A0<= /font>

=

Phone: +91 2= 0-3058-9517
Mobile: +91 976-788-8246
<= /div>






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

<= img src=3D"http://www.enterprisedb.com/sites/default/files/EDB-logo-4c.png"= width=3D"96" height=3D"54">





--
Akshay Joshi
Principal Software Engineer=C2=A0
<= br>

--001a114a9b66302f77054f856d5b--