Hi Dave,
On Tue, Dec 5, 2017 at 9:43 AM, Dave Page <dp...@pgadmin.org> wrote:
> Hi
>
> On Mon, Dec 4, 2017 at 4:01 PM, Khushboo Vashi <
> khushboo.va...@enterprisedb.com> wrote:
>
>>
>>
>> On Sat, Dec 2, 2017 at 10:42 AM, Dave Page <dp...@pgadmin.org> wrote:
>>
>>> Hi
>>>
>>> On Thursday, November 30, 2017, Khushboo Vashi <
>>> khushboo.va...@enterprisedb.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> Please find the attached updated patch.
>>>>
>>>> On Mon, Nov 27, 2017 at 5:15 PM, Dave Page <dp...@pgadmin.org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Thu, Nov 23, 2017 at 1:28 PM, Khushboo Vashi <
>>>>> khushboo.va...@enterprisedb.com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please find the attached patch for RM #2849: Allow editing of data on
>>>>>> tables with OIDs but no primary key.
>>>>>>
>>>>>
>>>>> I like that if I add a new row or rows and hit Save, the OIDs are
>>>>> fetched immediately. However;
>>>>>
>>>>> - It marks the row as read-only. We do that currently because we don't
>>>>> return the key info on Save, and thus require a Refresh before any further
>>>>> editing. However, if we have the OID, we can edit again immediately.
>>>>>
>>>>> Fixed
>>>>
>>>>> - If we can return the new OIDs on Save, can't we do the same for
>>>>> primary key values? That would be consistent with OIDs, and would remove
>>>>> the need to disable rows, leading to a much nicer use experience I think.
>>>>>
>>>>> Implemented
>>>>
>>>
>>> This looks great, however there is one small issue I spotted; it looks
>>> like we're inserting rows in a random order. For example, in the screenshot
>>> attached, the last 5 rows were added in the order seen in the key1 column,
>>> and then I hit Save and got the id values returned. Is that caused by
>>> something we're doing, or the database server?
>>>
>>> The root cause of the issue is, Python dictionary does not preserve the
>> order. To fix this issue I have manually sorted the added rows index and
>> stored it into OrderedDict.
>> Please find the attached updated patch.
>>
>
> Thanks Khushboo. My apologies, but I found something else when testing.
> Instead of just returning and updating the values for the key columns, we
> should do it for all columns. This would have 2 benefits (and I suspect,
> might actually make the code a little more simple):
>
> Done
> 1) We'd see the values for columns with default values.
>
> 2) We'd see the formatted values for other columns - e.g. with a JSONB
> column, you'll immediately see what the re-generated JSON looks like.
>
> I assume it's straightforward to update all columns rather than just the
> key columns?
>
The approach is same. Before I was just updating the primary keys/oids, now
I update all the columns of a row.
Please find the attached updated patch.
> Thanks!
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
Thanks,
Khushboo
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql
index f3353d6..2c3e573 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/9.2_plus/nodes.sql
@@ -15,9 +15,13 @@ WHERE
{% if clid %}
AND att.attnum = {{ clid|qtLiteral }}
{% endif %}
- {### To show system objects ###}
- {% if not show_sys_objects %}
+{### To show system objects ###}
+{% if not show_sys_objects and not has_oids %}
AND att.attnum > 0
- {% endif %}
+{% endif %}
+{### To show oids in view data ###}
+{% if has_oids %}
+ AND (att.attnum > 0 OR (att.attname = 'oid' AND att.attnum < 0))
+{% endif %}
AND att.attisdropped IS FALSE
ORDER BY att.attnum
diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql
index 4f1de2a..584f7b1 100644
--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/column/sql/default/nodes.sql
@@ -16,8 +16,12 @@ WHERE
AND att.attnum = {{ clid|qtLiteral }}
{% endif %}
{### To show system objects ###}
-{% if not show_sys_objects %}
+{% if not show_sys_objects and not has_oids %}
AND att.attnum > 0
{% endif %}
+{### To show oids in view data ###}
+{% if has_oids %}
+ AND (att.attnum > 0 OR (att.attname = 'oid' AND att.attnum < 0))
+{% endif %}
AND att.attisdropped IS FALSE
ORDER BY att.attnum
diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py
index 8dcb444..363d6f8 100644
--- a/web/pgadmin/tools/sqleditor/__init__.py
+++ b/web/pgadmin/tools/sqleditor/__init__.py
@@ -433,6 +433,9 @@ def start_view_data(trans_id):
sql = trans_obj.get_sql()
pk_names, primary_keys = trans_obj.get_primary_keys(default_conn)
+ # Fetch OIDs status
+ has_oids = trans_obj.has_oids(default_conn)
+
# Fetch the applied filter.
filter_applied = trans_obj.is_filter_applied()
@@ -444,6 +447,10 @@ def start_view_data(trans_id):
# Store the primary keys to the session object
session_obj['primary_keys'] = primary_keys
+
+ # Store the OIDs status into session object
+ session_obj['has_oids'] = has_oids
+
update_session_grid_transaction(trans_id, session_obj)
# Execute sql asynchronously
@@ -655,6 +662,8 @@ def poll(trans_id):
types = {}
client_primary_key = None
rset = None
+ has_oids = False
+ oids = None
# Check the transaction and connection status
status, error_msg, conn, trans_obj, session_obj = check_transaction_status(trans_id)
@@ -680,6 +689,11 @@ def poll(trans_id):
if 'primary_keys' in session_obj:
primary_keys = session_obj['primary_keys']
+ if 'has_oids' in session_obj:
+ has_oids = session_obj['has_oids']
+ if has_oids:
+ oids = {'oid': 'oid'}
+
# Fetch column information
columns_info = conn.get_column_info()
client_primary_key = generate_client_primary_key_name(
@@ -698,7 +712,8 @@ def poll(trans_id):
SQL = render_template("/".join([template_path,
'nodes.sql']),
- tid=command_obj.obj_id)
+ tid=command_obj.obj_id,
+ has_oids=True)
# rows with attribute not_null
colst, rset = conn.execute_2darray(SQL)
if not colst:
@@ -811,7 +826,9 @@ def poll(trans_id):
'colinfo': columns_info,
'primary_keys': primary_keys,
'types': types,
- 'client_primary_key': client_primary_key
+ 'client_primary_key': client_primary_key,
+ 'has_oids': has_oids,
+ 'oids': oids
}
)
@@ -945,7 +962,7 @@ def save(trans_id):
and trans_obj is not None and session_obj is not None:
# If there is no primary key found then return from the function.
- if len(session_obj['primary_keys']) <= 0 or len(changed_data) <= 0:
+ if (len(session_obj['primary_keys']) <= 0 or len(changed_data) <= 0) and 'has_oids' not in session_obj:
return make_json_response(
data={
'status': False,
diff --git a/web/pgadmin/tools/sqleditor/command.py b/web/pgadmin/tools/sqleditor/command.py
index d09bf2b..70db0c9 100644
--- a/web/pgadmin/tools/sqleditor/command.py
+++ b/web/pgadmin/tools/sqleditor/command.py
@@ -364,16 +364,20 @@ class TableCommand(GridCommand):
# Fetch the primary keys for the table
pk_names, primary_keys = self.get_primary_keys(default_conn)
+ # Fetch OIDs status
+ has_oids = self.has_oids(default_conn)
+
sql_filter = self.get_filter()
if sql_filter is None:
sql = render_template("/".join([self.sql_path, 'objectquery.sql']), object_name=self.object_name,
nsp_name=self.nsp_name, pk_names=pk_names, cmd_type=self.cmd_type,
- limit=self.limit, primary_keys=primary_keys)
+ limit=self.limit, primary_keys=primary_keys, has_oids=has_oids)
else:
sql = render_template("/".join([self.sql_path, 'objectquery.sql']), object_name=self.object_name,
nsp_name=self.nsp_name, pk_names=pk_names, cmd_type=self.cmd_type,
- sql_filter=sql_filter, limit=self.limit, primary_keys=primary_keys)
+ sql_filter=sql_filter, limit=self.limit, primary_keys=primary_keys,
+ has_oids=has_oids)
return sql
@@ -418,6 +422,31 @@ class TableCommand(GridCommand):
def can_filter(self):
return True
+ def has_oids(self, default_conn=None):
+ """
+ This function checks whether the table has oids or not.
+ """
+ driver = get_driver(PG_DEFAULT_DRIVER)
+ if default_conn is None:
+ manager = driver.connection_manager(self.sid)
+ conn = manager.connection(did=self.did, conn_id=self.conn_id)
+ else:
+ conn = default_conn
+
+ if conn.connected():
+
+ # Fetch the table oids status
+ query = render_template("/".join([self.sql_path, 'has_oids.sql']), obj_id=self.obj_id)
+
+ status, has_oids = conn.execute_scalar(query)
+ if not status:
+ raise Exception(has_oids)
+
+ else:
+ raise Exception(gettext('Not connected to server or connection with the server has been closed.'))
+
+ return has_oids
+
def save(self,
changed_data,
columns_info,
@@ -464,6 +493,7 @@ class TableCommand(GridCommand):
if len(changed_data[of_type]) < 1:
continue
+
column_type = {}
column_data = {}
for each_col in columns_info:
@@ -481,6 +511,12 @@ class TableCommand(GridCommand):
# For newly added rows
if of_type == 'added':
+ # Python dict does not honour the inserted item order
+ # So to insert data in the order, we need to make ordered list of added index
+ # We don't need this mechanism in updated/deleted rows as
+ # it does not matter in those operations
+ added_index = OrderedDict(sorted(changed_data['added_index'].items(),
+ key=lambda x: int(x[0])))
list_of_sql[of_type] = []
# When new rows are added, only changed columns data is
@@ -489,9 +525,12 @@ class TableCommand(GridCommand):
# of not null which is set by default.
column_data = {}
pk_names, primary_keys = self.get_primary_keys()
+ has_oids = 'oid' in column_type
- for each_row in changed_data[of_type]:
- data = changed_data[of_type][each_row]['data']
+ for each_row in added_index:
+ # Get the row index to match with the added rows dict key
+ tmp_row_index = added_index[each_row]
+ data = changed_data[of_type][tmp_row_index]['data']
# Remove our unique tracking key
data.pop(client_primary_key, None)
data.pop('is_row_copied', None)
@@ -507,8 +546,16 @@ class TableCommand(GridCommand):
object_name=self.object_name,
nsp_name=self.nsp_name,
data_type=column_type,
- pk_names=pk_names)
- list_of_sql[of_type].append({'sql': sql, 'data': data})
+ pk_names=pk_names,
+ has_oids=has_oids)
+ select_sql = render_template("/".join([self.sql_path, 'select.sql']),
+ object_name=self.object_name,
+ nsp_name=self.nsp_name,
+ pk_names=pk_names.split(",") if pk_names else None,
+ has_oids=has_oids)
+ list_of_sql[of_type].append({'sql': sql, 'data': data,
+ 'client_row': tmp_row_index,
+ 'select_sql': select_sql})
# Reset column data
column_data = {}
@@ -568,13 +615,15 @@ class TableCommand(GridCommand):
for opr, sqls in list_of_sql.items():
for item in sqls:
if item['sql']:
- status, res = conn.execute_void(
- item['sql'], item['data'])
- rows_affected = conn.rows_affected()
+ row_added = None
- # store the result of each query in dictionary
- query_res[count] = {'status': status, 'result': res,
- 'sql': sql, 'rows_affected': rows_affected}
+ # Fetch oids/primary keys
+ if 'select_sql' in item and item['select_sql']:
+ status, res = conn.execute_dict(
+ item['sql'], item['data'])
+ else:
+ status, res = conn.execute_void(
+ item['sql'], item['data'])
if not status:
conn.execute_void('ROLLBACK;')
@@ -591,6 +640,38 @@ class TableCommand(GridCommand):
_rowid = 0
return status, res, query_res, _rowid
+
+ # Select added row from the table
+ if 'select_sql' in item:
+ status, sel_res = conn.execute_dict(
+ item['select_sql'], res['rows'][0])
+
+ if not status:
+ conn.execute_void('ROLLBACK;')
+ # If we roll backed every thing then update the message for
+ # each sql query.
+ for val in query_res:
+ if query_res[val]['status']:
+ query_res[val]['result'] = 'Transaction ROLLBACK'
+
+ # If list is empty set rowid to 1
+ try:
+ _rowid = list_of_rowid[count] if list_of_rowid else 1
+ except Exception:
+ _rowid = 0
+
+ return status, sel_res, query_res, _rowid
+
+ if 'rows' in sel_res and len(sel_res['rows']) > 0:
+ row_added = {item['client_row']: sel_res['rows'][0]}
+
+ rows_affected = conn.rows_affected()
+
+ # store the result of each query in dictionary
+ query_res[count] = {'status': status, 'result': None if row_added else res,
+ 'sql': sql, 'rows_affected': rows_affected,
+ 'row_added': row_added}
+
count += 1
# Commit the transaction if there is no error found
diff --git a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
index 8c1a82c..a773b82 100644
--- a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
+++ b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
@@ -599,7 +599,10 @@ define('tools.querytool', [
options['width'] = column_size[table_name][c.name];
}
// If grid is editable then add editor else make it readonly
- if (c.cell == 'Json') {
+ if (c.cell == 'oid') {
+ options['editor'] = null;
+ }
+ else if (c.cell == 'Json') {
options['editor'] = is_editable ? Slick.Editors.JsonText
: Slick.Editors.ReadOnlyJsonText;
options['formatter'] = c.is_array ? Slick.Formatters.JsonStringArray : Slick.Formatters.JsonString;
@@ -688,13 +691,14 @@ define('tools.querytool', [
grid.registerPlugin(gridSelector);
var editor_data = {
- keys: self.handler.primary_keys,
+ keys: (_.isEmpty(self.handler.primary_keys) && self.handler.has_oids) ? self.handler.oids : self.handler.primary_keys,
vals: collection,
columns: columns,
grid: grid,
selection: grid.getSelectionModel(),
editor: self,
- client_primary_key: self.client_primary_key
+ client_primary_key: self.client_primary_key,
+ has_oids: self.handler.has_oids
};
self.handler.slickgrid = grid;
@@ -820,7 +824,8 @@ define('tools.querytool', [
// self.handler.data_store.updated will holds all the updated data
var changed_column = args.grid.getColumns()[args.cell].field,
updated_data = args.item[changed_column], // New value for current field
- _pk = args.item[self.client_primary_key] || null, // Unique key to identify row
+ _pk = args.item[self.client_primary_key] || null, // Unique key to identify row
+ has_oids = self.handler.has_oids || null, // Unique key to identify row
column_data = {},
_type;
@@ -852,6 +857,7 @@ define('tools.querytool', [
column_data[changed_column] = updated_data;
+
if (_pk) {
// Check if it is in newly added row by user?
if (_pk in self.handler.data_store.added) {
@@ -859,7 +865,7 @@ define('tools.querytool', [
self.handler.data_store.added[_pk]['data'],
column_data);
//Find type for current column
- self.handler.data_store.added[_pk]['err'] = false
+ self.handler.data_store.added[_pk]['err'] = false;
// Check if it is updated data from existing rows?
} else if (_pk in self.handler.data_store.updated) {
_.extend(
@@ -1900,18 +1906,20 @@ define('tools.querytool', [
_render: function (data) {
var self = this;
self.colinfo = data.col_info;
- self.primary_keys = data.primary_keys;
+ self.primary_keys = (_.isEmpty(data.primary_keys) && data.has_oids)? data.oids : data.primary_keys;
self.client_primary_key = data.client_primary_key;
self.cell_selected = false;
self.selected_model = null;
self.changedModels = [];
+ self.has_oids = data.has_oids;
+ self.oids = data.oids;
$('.sql-editor-explain').empty();
/* If object don't have primary keys then set the
* can_edit flag to false.
*/
- if (self.primary_keys === null || self.primary_keys === undefined
- || _.size(self.primary_keys) === 0)
+ if ((self.primary_keys === null || self.primary_keys === undefined
+ || _.size(self.primary_keys) === 0) && self.has_oids == false)
self.can_edit = false;
else
self.can_edit = true;
@@ -2072,6 +2080,9 @@ define('tools.querytool', [
}
// Identify cell type of column.
switch (type) {
+ case "oid":
+ col_cell = 'oid';
+ break;
case "json":
case "json[]":
case "jsonb":
@@ -2128,7 +2139,7 @@ define('tools.querytool', [
'pos': c.pos,
'label': column_label,
'cell': col_cell,
- 'can_edit': self.can_edit,
+ 'can_edit': (c.name == 'oid') ? false : self.can_edit,
'type': type,
'not_null': c.not_null,
'has_default_val': c.has_default_val,
@@ -2379,7 +2390,29 @@ define('tools.querytool', [
dataView = grid.getData(),
data_length = dataView.getLength(),
data = [];
+
if (res.data.status) {
+ if(is_added) {
+ // Update the rows in a grid after addition
+ dataView.beginUpdate();
+ _.each(res.data.query_result, function (r) {
+ if(!_.isNull(r.row_added)) {
+ // Fetch temp_id returned by server after addition
+ var row_id = Object.keys(r.row_added)[0];
+ _.each(req_data.added_index, function(v, k) {
+ if (v == row_id) {
+ // Fetch item data through row index
+ var item = grid.getDataItem(k);
+ _.each(r.row_added[row_id], function (p, k) {
+ // Update the column values if different
+ if(item[k] != p) item[k] = p;
+ })
+ }
+ })
+ }
+ });
+ dataView.endUpdate();
+ }
// Remove flag is_row_copied from copied rows
_.each(data, function (row, idx) {
if (row.is_row_copied) {
@@ -2412,15 +2445,6 @@ define('tools.querytool', [
grid.setSelectedRows([]);
}
- // whether a cell is editable or not is decided in
- // grid.onBeforeEditCell function (on cell click)
- // but this function should do its job after save
- // operation. So assign list of added rows to original
- // rows_to_disable array.
- if (is_added) {
- self.rows_to_disable = _.clone(self.temp_new_rows);
- }
-
grid.setSelectedRows([]);
// Reset data store
diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/has_oids.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/has_oids.sql
new file mode 100644
index 0000000..edeeb83
--- /dev/null
+++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/has_oids.sql
@@ -0,0 +1,6 @@
+{# ============= Check object has OIDs or not ============= #}
+{% if obj_id %}
+SELECT rel.relhasoids AS has_oids
+FROM pg_class rel
+WHERE rel.oid = {{ obj_id }}::oid
+{% endif %}
diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/insert.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/insert.sql
index 23ffcb4..a4ed872 100644
--- a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/insert.sql
+++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/insert.sql
@@ -5,4 +5,6 @@ INSERT INTO {{ conn|qtIdent(nsp_name, object_name) }} (
) VALUES (
{% for col in data_to_be_saved %}
{% if not loop.first %}, {% endif %}%({{ col }})s::{{ data_type[col] }}{% endfor %}
-);
+)
+{% if pk_names %} returning {{pk_names}}{% endif %}
+{% if has_oids %} returning oid{% endif %};
diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/objectquery.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/objectquery.sql
index 2c6ba58..1cb60d9 100644
--- a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/objectquery.sql
+++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/objectquery.sql
@@ -1,5 +1,5 @@
{# SQL query for objects #}
-SELECT * FROM {{ conn|qtIdent(nsp_name, object_name) }}
+SELECT {% if has_oids %}oid, {% endif %}* FROM {{ conn|qtIdent(nsp_name, object_name) }}
{% if sql_filter %}
WHERE {{ sql_filter }}
{% endif %}
diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/select.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/select.sql
new file mode 100644
index 0000000..45f364a
--- /dev/null
+++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/select.sql
@@ -0,0 +1,9 @@
+{# Select table rows #}
+SELECT {% if has_oids %}oid, {% endif %}* FROM {{ conn|qtIdent(nsp_name, object_name) }}
+WHERE
+{% if has_oids %}
+ oid = %(oid)s
+{% elif pk_names %}
+ {% for pk in pk_names %}
+ {% if not loop.first %} AND {% endif %}{{ conn|qtIdent(pk) }} = %({{ pk }})s{% endfor %}
+{% endif %};
