Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1haj2n-00088b-JZ for pgadmin-hackers@arkaria.postgresql.org; Tue, 11 Jun 2019 15:50:42 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1haj2m-0007cx-Dj for pgadmin-hackers@arkaria.postgresql.org; Tue, 11 Jun 2019 15:50: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_SHA1:256) (Exim 4.89) (envelope-from ) id 1haizc-0000PO-TJ for pgadmin-hackers@lists.postgresql.org; Tue, 11 Jun 2019 15:47:25 +0000 Received: from mail-it1-x144.google.com ([2607:f8b0:4864:20::144]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1haizZ-000706-Fu for pgadmin-hackers@postgresql.org; Tue, 11 Jun 2019 15:47:24 +0000 Received: by mail-it1-x144.google.com with SMTP id m138so5710927ita.4 for ; Tue, 11 Jun 2019 08:47:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=7vqmlpoHzIyUnvGzdUiBPGf3A72CltdwFuDghy1Mq7U=; b=huL/eF7q4BBVDoemcJd04aJ3w3kL7AczhY0iBIJimc9ZP/AZj8ZNX0N9FCE8/W+e7D 1S/nr6LHJ5yh144wXf6derFZzp6bBH5mmfBEB29ig0KjGfVbzG+aU5y2Ds9klJKhgAN0 O8pf/beRoKgTi3gR4WyExgNj+KuDu4rV51N6XeP7wMY1upzgBRZ4b6pvMsHQfj0i5Rf5 piNu4x9zrRVSbdwsw/Q8ILCZIPZ99E6JnDHUSkrHRErRQlamPcvz6Umd9hZYyn6iUwH5 tsqKFbkPEurBDuKMKBfgYxR/wWOLFlV/ptKUfLCB0XeqBPSM6nu7EKCdFqtHAj3P+Rsi FVbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=7vqmlpoHzIyUnvGzdUiBPGf3A72CltdwFuDghy1Mq7U=; b=PRCuOWztLdH7pS7Hflk03EjXyZMX1MAl+U4VoOlpdc/8O6uglVfxq+V+aOwXpm1GGY 31nEX94mqByxyESCaQOUaQ4cQlALQJWW1+GPagfOb44n028ehAzDDCLahTvygYn/PMTj 9SFS7OzcWl88lXt0y6fkiYN856iQ/WpzhfbSINosVMKcC1sXhiNgEwucybbsCmpvLx8H /89FYyZfpRFNGC8eBUqsSWtlS5X/8duYRlENAJIv3mY5u+ymVbtkNtdN90SmEVXuTnDY rzQIovaAA8+3GDmZBQIVbKCdabiFaYACHCW1PEpjM26W6O8jn+oRxnZBYj8Xrwod3JQZ Hs8g== X-Gm-Message-State: APjAAAUHFiYtH0evIq2pdVbrHqPUYmEBF5RPlF6rfenDro4JwHWEgRH4 +EXiVnU33FpDVOFbPFCecoOk7Kn9BEdnxMyCx/lBOw== X-Google-Smtp-Source: APXvYqx6nXxRaexGlVPcRgDXMYD5yeS5bj4pMyYCSwJ1jR2jZeIRgONz+edZB9emMmjWdBQlC3iPwZWEiFx7RnOkehM= X-Received: by 2002:a05:660c:c1:: with SMTP id q1mr17721518itk.103.1560268039597; Tue, 11 Jun 2019 08:47:19 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Tue, 11 Jun 2019 16:47:06 +0100 Message-ID: Subject: Re: [GSoC] Check if a query resultset is update-able To: Yosry Muhammad Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="00000000000041453a058b0e367b" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000041453a058b0e367b Content-Type: text/plain; charset="UTF-8" Hi On Tue, Jun 11, 2019 at 3:06 PM Yosry Muhammad wrote: > Hi, > > On Tue, Jun 11, 2019 at 10:21 AM Dave Page wrote: > >> Hi >> >> On Mon, Jun 10, 2019 at 10:51 PM Yosry Muhammad >> wrote: >> >>> Dear all, >>> >>> After some research and review of the code, I have arrived at this >>> method to check whether a query resultset is update-able. I would like to >>> know your feedback before I start implementing this technique: >>> >>> - Query results are not be update-able (initially at least) except if >>> all the primary keys are selected and all the columns belong to the same >>> table (no joins or aggregations). >>> >> >> What are all the conditions that prevent a query being updateable? Joins, >> aggregates, lack of pkey, computed columns, function calls....? >> >> >>> - When the query results is ready (polling is successful) I can check >>> the results in the back-end using the transaction connection cursor. >>> - The transaction cursor description attribute includes a list of Column >>> objects, each of which has attributes pointing to its original table in the >>> system catalog table *pg_class* (if available) and its attribute number >>> within the table. [1] >>> >> >> Hmm, at first glance it seems to me that psycopg2.extensions.Column might >> make this much easier than previously expected: >> >> - Is Column.table_oid the same for all columns? >> - Is Column.table_column present for all columns? >> >> then: >> >> - Do we have all the primary key columns in the resultset? >> >> In fact, we could even support queries such as: >> >> SELECT col1 || col2, col1, col2 FROM table; >> >> because (assuming col1 or col2 is the pkey) we can just make the first >> column read-only in the grid, and only allow editing of the second and >> third. That requires that the table_oid matches of course. >> > > According to my understanding, Column.table_oid is the same for all > columns belonging to the same table and is None if the column does not > directly reference an original column in a table (an aggregation or a > result of a mathematical operation or concatenation for example). It > follows that Column.table_column is only present for the columns that have > a table_oid attribute that isn't None. > > For the first version, the query resultset can be update-able if (and only > if): > - All the columns either belong to the same table or no tables (such as > the concatenation case you pointed out). > - All the primary keys of that column are available. > I assume you mean table, and not column. > columns that do not belong to the table can be made read-only. > > Please keep in mind that including columns that do not belong directly to > the table will require more modifications in the front-end part when > modifying the table (to drop any read-only columns and so). > I'd start by handling the "only columns from the table" case. Handling individual read-only columns can be handled if/when there is time. > > > > >> >> >>> - From this information, the system catalog tables *pg_class, >>> pg_attribute *and *pg_constraint *can be queried to check that all the >>> columns belong to a single table and that all the primary keys are >>> available. [2][3][4] >>> - This can be used as an indicator to whether the resultset is updatable >>> (similar to the View Table mode, where tables are only editable if they >>> have primary keys). >>> >>> I will modify the following parts in the code: >>> 1- *web/tools/sqleditor/command.py* >>> QueryToolCommand class will be modified to contain an attribute >>> indicating whether the query results are update-able for the last >>> successful query. >>> >>> 2- A new file will be added in* web/tools/sqleditor/utils/* containing >>> the function that will check if the query results are update-able. >>> >>> 3- >>> *web/tools/sqleditor/__init__.py * >>> The poll endpoint will be modified to check if the results are >>> update-able (in case the results are ready), then the session object >>> primary keys and the transaction object can_edit attribute will be updated >>> (the primary keys are checked in the frontend, if they exist table >>> modifications are allowed). >>> >> >> You mean the endpoint that is polled for the results, or the transaction >> status poll endpoint? I'm assuming the former. >> > > I did mean the endpoint that is polled for the results indeed. > :-) > > >> >> >>> >>> This is the first step, to check if a query resultset is update-able. >>> The upcoming steps will include switching the mode in the frontend to allow >>> for editing the results and checking what options should be enabled or >>> disabled and any needed modifications (I think allowing for only editing >>> and deleting rows makes sense). >>> >> >> I don't see any obvious reason why we couldn't add rows as well. Of >> course, they may fail if a not null column isn't present in the query, but >> the user can fix that (we should probably not just make the grid read-only >> if we detect that, unless we can come up with a nice way to tell the user >> why they can't edit anything). >> > > Inserting the rows can be supported I didn't look into this part yet, was > just letting you know of the upcoming steps in my point of view. For now I > will work on the aforementioned implementation. It can be extended later > according to what you see suitable, for example allow editing result-sets > containing columns from different tables). > > Looking forward for your feedback to start working on that at once. > I would suggest working on IUD for resultsets that are entirely from a single table. I think that insert is as important as update and delete, and less important that supporting cases like the example I gave. Otherwise, sounds good to me! > > > >> >> >>> >>> Sorry for the long email, looking forward to your feedback! >>> >>> [1] >>> http://initd.org/psycopg/docs/extensions.html#psycopg2.extensions.Column >>> [2] https://www.postgresql.org/docs/current/catalog-pg-class.html >>> [3] https://www.postgresql.org/docs/current/catalog-pg-attribute.html >>> [4] https://www.postgresql.org/docs/current/catalog-pg-constraint.html >>> >>> -- >>> *Yosry Muhammad Yosry* >>> >>> Computer Engineering student, >>> The Faculty of Engineering, >>> Cairo University (2021). >>> Class representative of CMP 2021. >>> https://www.linkedin.com/in/yosrym93/ >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > > -- > *Yosry Muhammad Yosry* > > Computer Engineering student, > The Faculty of Engineering, > Cairo University (2021). > Class representative of CMP 2021. > https://www.linkedin.com/in/yosrym93/ > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --00000000000041453a058b0e367b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Tue, Jun 11, 2019 at 3:06 PM Yosry Muhammad <= yosrym93@gmail.com> wrote:
=
Hi,

On Tue, Jun 11, 2019 at 10:21 AM Dave Page <dpage@pgadmin.org> wrote:
=
Hi<= /div>
O= n Mon, Jun 10, 2019 at 10:51 PM Yosry Muhammad <yosrym93@gmail.com> wrote:
=
Dea= r all,

After some research and review of the code,= I have arrived at this method to check whether a query resultset is update= -able. I would like to know your feedback before I start implementing this = technique:

- Query results are not be update-able = (initially at least) except if all the primary keys are selected and all th= e columns belong to the same table (no joins or aggregations).
<= /blockquote>

What are all the conditions that prevent a = query being updateable? Joins, aggregates, lack of pkey, computed columns, = function calls....?
=C2=A0
- When the query results is ready (p= olling is successful) I can check the results in the back-end using the tra= nsaction connection cursor.
- The transaction cursor description = attribute includes a list of Column objects, each of which has attributes p= ointing to its original table in the system catalog table pg_class (= if available) and its attribute number within the table. [1]

Hmm, at first glance it seems to me that psyc= opg2.extensions.Column might make this much easier than previously expected= :

- Is Column.table_oid the same for all columns?<= /div>
- Is Column.table_column present for all columns?

<= /div>
then:

- Do we have all the primary key c= olumns in the resultset?

In fact, we could even su= pport queries such as:

SELECT col1 || col2, col1, = col2 FROM table;

because (assuming col1 or col2 is= the pkey) we can just make the first column read-only in the grid, and onl= y allow editing of the second and third. That requires that the table_oid m= atches of course.

Ac= cording to my understanding, Column.table_oid=20 is the same for all columns belonging to the same table and is None if=20 the column does not directly reference an original column in a table (an aggregation or a result of a mathematical operation or concatenation=20 for example). It follows that Column.table_column is only present for=20 the columns that have a table_oid attribute that isn't None.
=
For the first version, the query resultset can be update-abl= e if (and only if):
- All the columns either belong to the same t= able or no tables (such as the concatenation case you pointed out).
- All the primary keys of that column are available.

I assume you mean table, and not colum= n.
=C2=A0
=
columns that do not b= elong to the table can be made read-only.

Please keep in mind that including columns that do not belong directly to the=20 table will require more modifications in the front-end part when=20 modifying the table (to drop any read-only columns and so).

I'd start by handling the &quo= t;only columns from the table" case. Handling individual read-only col= umns can be handled if/when there is time.
=C2=A0


=C2=A0
=C2=A0
- From this information, the system catalog tables pg_cl= ass, pg_attribute and pg_constraint can be queried to check that= all the columns belong to a single table and that all the primary keys are= available. [2][3][4]
- This can be used as an indicator to w= hether the resultset is updatable (similar to the View Table mode, where ta= bles are only editable if they have primary keys).

=
I will modify the following parts in the code:
1- web/too= ls/sqleditor/command.py
QueryToolCommand class will be mo= dified to contain an attribute indicating whether the query results are upd= ate-able for the last successful query.

2- A new f= ile will be added in web/tools/sqleditor/utils/ containing the funct= ion that will check if the query results are update-able.

3- web/tools/sqleditor/__init__.py
The poll e= ndpoint will be modified to check if the results are update-able (in case t= he results are ready), then the session object primary keys and the transac= tion object can_edit attribute will be updated (the primary keys are checke= d in the frontend, if they exist table modifications are allowed).

You mean the endpoint that is polled fo= r the results, or the transaction status poll endpoint? I'm assuming th= e former.

I did mean= the endpoint that is polled for the results indeed.

:-)
=C2=A0
=C2=A0
=C2=A0

This = is the first step, to check if a query resultset is update-able. The upcomi= ng steps will include switching the mode in the frontend to allow for editi= ng the results and checking what options should be enabled or disabled and = any needed modifications (I think allowing for only editing and deleting ro= ws makes sense).

I don't se= e any obvious reason why we couldn't add rows as well. Of course, they = may fail if a not null column isn't present in the query, but the user = can fix that (we should probably not just make the grid read-only if we det= ect that, unless we can come up with a nice way to tell the user why they c= an't edit anything).

=
Inserting the rows can be supported I didn't=20 look into this part yet, was just letting you know of the upcoming steps in my point of view. For now I will work on the aforementioned=20 implementation. It can be extended later according to what you see=20 suitable, for example allow editing result-sets containing columns from=20 different tables).

Looking forward for your feedba= ck to start working on that at once.

I would suggest working on IUD for resultsets that are en= tirely from a single table. I think that insert is as important as update a= nd delete, and less important that supporting cases like the example I gave= .

Otherwise, sounds good to me!
=C2=A0

=C2=A0
<= div>=C2=A0

Sorry for the long email, looking forward to y= our feedback!


--
Yosry Muhammad Yosry

Computer Engineer= ing student,
The Faculty of Engineerin= g,
Cairo University (2021).
Class representative of CMP 2021.
<= /div>


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pg= snake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL C= ompany


--
Yosry Muhammad Yosry

Computer Engineering student,
The Faculty of Engineering,
Cairo U= niversity (2021).
Class representative = of CMP 2021.


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @p= gsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL = Company
--00000000000041453a058b0e367b--