Re: [pgAdmin4] [Patch]: Foreign Table Module

Hi,

Please find the attached updated patch for the Foreign Tables module.

Thanks,
Khushboo

On Mon, May 9, 2016 at 1:46 PM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

>
> Hi Khushboo,
>
> Please find comments as below,
> (Tested with Python3)
>
> *1)* Default string value is not quoted properly which causing SQL to
> break, here default string should be 'my test string' for myCol2.
>
> CREATE FOREIGN TABLE "Test123"."test1_Server"
> ("myCol" bigint NOT NULL DEFAULT 213 COLLATE pg_catalog."C",
> "myCol2" character varying(50) NOT NULL DEFAULT my test string COLLATE
> pg_catalog."C.UTF-8")
> INHERITS ("Test123".abc, pem.chart)
> SERVER test_fw_server;
>
> I think this behavior is perfect as we can't add single quotes for all the
data-types. User should have to add himself if required.

> *2)* When Length & Precision, does not clear when user selects another
> data type
> - First select Numeric provide Length & Precision and then choose abstime
> data type which have neither Length & Precision
> - It will not clear/ not allow user to clear old values & generates wrong
> SQL
>
> ALTER FOREIGN TABLE "Test123"."test1_TT1"
> ADD COLUMN col1 abstime(50 , 2) NOT NULL COLLATE pg_catalog."C";
>
Done

> *3)* I am allowed to select self table as inherited table
>
> ALTER FOREIGN TABLE pem."test1_TT1" INHERIT pem."test1_TT1";
>
> Done

> *4)* Wrong SQL generated for array like data types
>
> ALTER FOREIGN TABLE pem."test1_TT1"
> ADD COLUMN name character[](50 ) NOT NULL COLLATE pg_catalog."POSIX";
>
> Correct SQL:
>
> ALTER FOREIGN TABLE pem."test1_TT1"
> ADD COLUMN name character(50)[] NOT NULL COLLATE pg_catalog."POSIX";
>
> Done

> *5)* I am allowed to enter duplicate options but as per postgres
> documentation
> we should not allow duplicate options
>
> ALTER FOREIGN TABLE pem."test1_TT1"
> OPTIONS (ADD op1 'val1');
>
> ALTER FOREIGN TABLE pem."test1_TT1"
> OPTIONS (ADD op1 'val1');
>
> ALTER FOREIGN TABLE pem."test1_TT1"
> OPTIONS (ADD op1 'val2');
>
> Done

> *6)* Created table with Special name (eg: table name => "@Test#" ) & it
> breaks when we clicks on it.
>
> File
> "/home/murtuza/projects/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/__init__.py",
> line 414, in properties
> data = self._fetch_properties(gid, sid, did, scid, foid)
> File
> "/home/murtuza/projects/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/__init__.py",
> line 982, in _fetch_properties
> c['typlen'] = int(typlen)
> TypeError: int() argument must be a string, a bytes-like object or a
> number, not 'list'
>

>
Couldn't reproduce but I think while fixing above issues it is resolved.

> *7) *While dropping any foreign table gives this error but table gets
> deleted from browser tree.
>
> s/databases/schemas/foreign_tables/__init__.py", line 414, in properties
> data = self._fetch_properties(gid, sid, did, scid, foid)
> File
> "/home/murtuza/projects/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/foreign_tables/__init__.py",
> line 982, in _fetch_properties
> c['typlen'] = int(typlen)
> TypeError: int() argument must be a string, a bytes-like object or a
> number, not 'list'
>
Couldn't reproduce but I think while fixing above issues it is resolved.

>
> *Can be added into TODO:*
> *=====================*
>
> *1)* We should not allowed user to Inherits from catalog tables like
> pg_catalog.pg_type;
>
> *2)* Minor SQL alignment when only server & table name was given
>
> CREATE FOREIGN TABLE "Test123"."test1_Server"
> ()
> SERVER test_fw_server;
>
> Expected:
> ---------
> CREATE FOREIGN TABLE "Test123"."test1_Server" ()
> SERVER test_fw_server;
>
> *3)* Collation client side validation missing for columns, not every data
> type support Collations,
> For other data types it should be disable just like Length & Precision.
>
>
> Regards,
> Murtuza
>
>
>
> On 09-May-2016, at 10:43 am, Murtuza Zabuawala <
> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>
> Hi Khushboo,
>
> No I did not. let me apply it and try again.
>
> Thanks,
> Murtuza
>
>
> On 06-May-2016, at 10:15 pm, Khushboo Vashi <
> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>
> Hi Murtuza,
>
> Have you applied Dependent Cell patch on this? As the Foreign table is
> dependent on that.
>
> Thanks,
> Khushboo
> On 6 May 2016 20:39, "Murtuza Zabuawala" <
> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>
>> Hi Khushboo,
>>
>> Please find comments as below,
>>
>> I pulled latest version of code and then I applied v2 patch.
>>
>>
>> 1) Once applying patch, When I re-started pgAdmin4 server again, I got
>> below error (Screenshot is also attached),
>> * ( FYI, I was not able to re-produce it again second time. )*
>>
>>
>>
>>
>>
>> * File "/Users/murtuza/Desktop/pgadmin4/web/pgadmin/utils/__init__.py",
>> line 37, in create_module_preference self.preference =
>> Preferences(self.name <http://self.name/>, None) File
>> "/Users/murtuza/Desktop/pgadmin4/web/pgadmin/utils/preferences.py", line
>> 261, in __init__ db.session.commit()…..*
>>
>> *…..*
>>
>> * cursor.execute(statement,
>> parameters)sqlalchemy.exc.OperationalError: (OperationalError) database is
>> locked 'INSERT INTO module_preference (name) VALUES (?)'
>> ('NODE-foreign-table’,)*
>>
>>
>> 2) I am not able to create/open Foreign table node as I am getting errors
>> from JS side (when I expand the schema node & do not get Create context
>> menu as well)
>> Please find screenshots for both scenario.
>>
>>
>> <Screenshot from 2016-05-06 08-00-19.png><Screenshot from 2016-05-06
>> 07-59-45.png><Screen Shot 2016-05-06 at 8.18.12 pm.png>
>>
>>
>> On 06-May-2016, at 3:55 pm, Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>> Hi,
>>
>> Please find the attached patch for the Foreign Table Node with fixed
>> issues.
>>
>> Thanks,
>> Khushboo
>>
>> On Fri, Apr 29, 2016 at 12:20 PM, Neel Patel <neel(dot)patel(at)enterprisedb(dot)com
>> > wrote:
>>
>>> Hi Khushboo,
>>>
>>> Below are the observations.
>>>
>>> - When we create the new Foreign Table with column name and types
>>> then it shows NULL along with column name and type in properties tab.
>>>
>>> e.g. column_1 xml[] NULL
>>>
>> I think if its NOT NULL, then it should be NULL and it is default, so
>> this should be okay.
>>
>>>
>>> - Once we inherits the table from another table then column and
>>> another parameters of inherited table should not allowed to change.
>>>
>>> Done
>>
>>>
>>> - When we create any foreign table then same foreign table is also
>>> listed under "Tables" node.
>>>
>>> This bug is related to Table node and Harshal is working on this issue.
>>
>>>
>>> - SQL is not generated properly. Please find below SQL which gives
>>> error during execution.
>>>
>>> CREATE FOREIGN TABLE public.test_2
>>> (id1 integer NOT NULL DEFAULT12 COLLATEpg_catalog."POSIX")
>>> SERVER fsrv;
>>>
>> Done
>>
>>>
>>> - When we create the new foreign table with security label then no
>>> SQL is generated for security label.
>>>
>>> Done
>>
>>>
>>> - In Edit mode, when we provide security label with both value
>>> "provider" and "security label" then security label is displayed NULL
>>>
>>> e.g. SECURITY LABEL FOR frgn_table ON FOREIGN TABLE
>>> public.fsrv_table IS NULL;
>>>
>> Done
>>
>>>
>>> - During creation of the column, when we remove the collation then
>>> it gives below error.
>>>
>>> TypeError: item is undefined
>>>
>> Done
>>
>>>
>>> - Delete/Drop cascade functionality is not working, we are getting
>>> below error.
>>>
>>> TypeError: self.canDrop.apply is not a function
>>>
>> Done
>>
>>>
>>> - When we edit the foreign table and try to remove the existing
>>> "Data Type" of column then it gives below error.
>>>
>>> TypeError: this.dataAdapter is null
>>>
>> Done
>>
>>>
>>> - Create the new foreign table and click on ADD button in Column tab
>>> and do not provide any column name and data type. Need to do proper
>>> validation in Column tab for all parameters. Currently if user do not
>>> provide any value then wrong SQL is getting generated.
>>>
>>> CREATE FOREIGN TABLE public.test_4
>>> (None None NULL)
>>> SERVER test_fsrv;
>>>
>> Done
>>
>>>
>>> - When we do not provide the Check parameters in constraint then it
>>> gives SQL syntax error.
>>>
>>> CREATE FOREIGN TABLE public.test_5
>>> ()
>>> SERVER test_fsrv;
>>>
>>> ALTER FOREIGN TABLE public.test_5
>>> ADD CONSTRAINT test CHECK () NOT VALID;
>>>
>> Done
>>
>>>
>>> - If we edit foreign table and change the schema then it gives below
>>> error.
>>>
>>> IndexError: list index out of range
>>>
>> Done
>>
>>>
>>> - We should have proper indentation in the SQL tab once we give the
>>> parameters. Currently it looks like below for "Options" value.
>>>
>>> CREATE FOREIGN TABLE "1_test"."5_test"
>>> ()
>>> SERVER asas
>>> OPTIONS (test1 'test2'
>>> , test3 'test4');
>>>
>> Done
>>
>>>
>>> - If user provide foreign table name and do not provide foreign
>>> server and click on SQL tab then we are getting error on browser side as
>>> below. We should have proper error handling in this case.
>>>
>>> Couldn't find the required parameter (ftsrvname).
>>>
>> Done
>>
>>>
>>> - Create the foreign table, add the constraint and do not provide
>>> any constraint information then SQL generated is wrong.
>>>
>>> CREATE FOREIGN TABLE "1_test"."9_test"
>>> ()
>>> SERVER test_fsrv;
>>>
>>> ALTER FOREIGN TABLE "1_test"."9_test"
>>> ADD CONSTRAINT None CHECK ();
>>>
>> Done
>>
>>>
>>> - When we click on the foreign table collection node then "Comment"
>>> column is blank even though we have comment in the foreign table.
>>>
>>> Done
>>
>>>
>>> - Create the foreign table on PG 9.1 and after pressing Save button
>>> we are getting below error.
>>>
>>> "the JSON object must be str, not 'list'"
>>>
>> Done
>>
>>>
>>> - When we delete the options parameters then it gives SQL error
>>> because DROP statement does not include the value.
>>>
>>> ALTER FOREIGN TABLE public.test_12
>>> OPTIONS ( DROP ser2 'val2');
>>>
>> Done
>>
>>>
>>> - There are some new functionality added in PG 9.5. Do we really
>>> need to implement ? Need to discuss with Dave/Ashesh. Below are the new
>>> functionality.
>>>
>>> - In create foreign table,we have added column constraint
>>> but "table constraint" is added from 9.5.Do <http://9.5.do/> we really
>>> require to add table constraint ?
>>> - In alter foreign table, below are the new functionality
>>> added.
>>> 1. ALTER [ COLUMN ] column_name SET STORAGE {
>>> PLAIN | EXTERNAL | EXTENDED | MAIN }
>>> 2. DISABLE TRIGGER [ trigger_name | ALL | USER ]
>>> 3. ENABLE TRIGGER [ trigger_name | ALL | USER ]
>>> 4. ENABLE REPLICA TRIGGER trigger_name
>>> 5. ENABLE ALWAYS TRIGGER trigger_name
>>> 6. SET WITH OIDS
>>> 7. SET WITHOUT OIDS
>>>
>>> As per the discussion, we will add these functionality into the next
>> phase.
>>
>>> Do let us know in case of any queries.
>>>
>>> Thanks,
>>> Neel Patel
>>>
>>> On Tue, Apr 5, 2016 at 2:27 PM, Khushboo Vashi <
>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi,
>>>>
>>>> Please find updated patch for the Foreign Table Module.
>>>>
>>>> This patch is dependent on
>>>> 1. Backgrid Depscell Patch, (submitted by me)
>>>> 2. NodeAjaxOptionsCell Transform change patch, on which Ashesh and
>>>> Murtuza are working
>>>>
>>>> Thanks,
>>>> Khushboo
>>>>
>>>>
>>>>
>>>>
>>>> On Wed, Feb 24, 2016 at 2:57 PM, Khushboo Vashi <
>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I have updated the Foreign Table module as below:
>>>>>
>>>>> - Used 'NodeByListControl' to get schemas, in the foreign_table.js
>>>>> file as suggested by Ashesh to avoid code redundancy.
>>>>>
>>>>> - Applied *'Security Label Macro'* Patch (Implemented by Harshal).
>>>>> To test the Foreign Table patch, 'Security Label Macro' patch must
>>>>> be applied first as that is not committed yet.
>>>>>
>>>>> Please find attached Foreign Table Patch.
>>>>>
>>>>> Thanks,
>>>>> Khushboo
>>>>>
>>>>> On Tue, Feb 23, 2016 at 6:53 PM, Khushboo Vashi <
>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please find attached patch for the Foreign Table Module.
>>>>>>
>>>>>> The patch will be modified after Types module implementation as we
>>>>>> need to populate Base Type and some Type related validations from the
>>>>>> Types module.
>>>>>>
>>>>>> Please review it and let me know the feedback.
>>>>>>
>>>>>> Thanks,
>>>>>> Khushboo
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>>>> To make changes to your subscription:
>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>>
>>>>
>>>
>> <pgAdmin4_Foreign_tables_ver2.patch>
>> --
>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>
>>
>>
>
>