Added kexi hotfix to support versions 12 and higher, fixing issue #15 #16

Merged
SlavekB merged 1 commits from feat/kexi-postgresql into master 5 months ago
ormorph commented 5 months ago
Collaborator

Added fixes for postgresql to support version 12 and higher. I checked that it seems to work even better than the driver for mysql. The mysql driver does not support tables without a primary key, but the postgresql driver does. The transition from hidden oid fields to ctid is performed. Checked everything seems to work.

Added fixes for postgresql to support version 12 and higher. I checked that it seems to work even better than the driver for mysql. The mysql driver does not support tables without a primary key, but the postgresql driver does. The transition from hidden oid fields to ctid is performed. Checked everything seems to work.
ormorph added 1 commit 5 months ago
SlavekB reviewed 5 months ago
SlavekB left a comment
Owner

I can not do testing functionality with PostgreSQL, but it seems good. There are some comments regarding the ability to better code readability. See comments below.

I can not do testing functionality with PostgreSQL, but it seems good. There are some comments regarding the ability to better code readability. See comments below.
if (m_driver->beh->ROW_ID_FIELD_NAME == "ctid") {
if (row_id<=0 || true!=querySingleRecord(
TQString::fromLatin1("SELECT ") + tableName + TQString::fromLatin1(".") + aiFieldName + TQString::fromLatin1(" FROM ")
+ tableName + TQString::fromLatin1(" WHERE ") + aiFieldName + TQString::fromLatin1("=") + TQString::number(row_id), rdata))
Owner

I understand that you left the style of code as it was used below, but I believe that there could be a better readable way:

if (row_id<=0 || true!=querySingleRecord(
	TQString("SELECT %1.%2 FROM %1 WHERE %2=%3").arg(tableName).arg(aiFieldName).arg(row_id), rdata))

If I correctly read the difference in the calls, there may be a single call in one condition:

if (row_id<=0 || true!=querySingleRecord(
	TQString("SELECT %1.%2 FROM %1 WHERE %3=%4").arg(tableName).arg(aiFieldName)
		.arg(m_driver->beh->ROW_ID_FIELD_NAME == "ctid" ? aiFieldName : m_driver->beh->ROW_ID_FIELD_NAME)
		.arg(row_id), rdata))
I understand that you left the style of code as it was used below, but I believe that there could be a better readable way: ``` if (row_id<=0 || true!=querySingleRecord( TQString("SELECT %1.%2 FROM %1 WHERE %2=%3").arg(tableName).arg(aiFieldName).arg(row_id), rdata)) ``` If I correctly read the difference in the calls, there may be a single call in one condition: ``` if (row_id<=0 || true!=querySingleRecord( TQString("SELECT %1.%2 FROM %1 WHERE %3=%4").arg(tableName).arg(aiFieldName) .arg(m_driver->beh->ROW_ID_FIELD_NAME == "ctid" ? aiFieldName : m_driver->beh->ROW_ID_FIELD_NAME) .arg(row_id), rdata)) ```
Poster
Collaborator

If I correctly read the difference in the calls, there may be a single call in one condition:

Oh sure. I'll deal with this later. While I checked this call, while it displays an error:

Error opening database cursor.
Message from server:
near "%": syntax error
SQL statement:
SELECT devel.OID FROM id WHERE 5=%4 LIMIT 1
Server result number:
1

Most likely, the arguments will have to be written in one line.

>If I correctly read the difference in the calls, there may be a single call in one condition: Oh sure. I'll deal with this later. While I checked this call, while it displays an error: ``` Error opening database cursor. Message from server: near "%": syntax error SQL statement: SELECT devel.OID FROM id WHERE 5=%4 LIMIT 1 Server result number: 1 ``` Most likely, the arguments will have to be written in one line.
Owner

My mistake – replacing argument cannot be used multiple times, so arguments need to be repeated:

if (row_id<=0 || true!=querySingleRecord(
	TQString("SELECT %1.%2 FROM %3 WHERE %4=%5").arg(tableName).arg(aiFieldName).arg(tableName)
		.arg(m_driver->beh->ROW_ID_FIELD_NAME == "ctid" ? aiFieldName : m_driver->beh->ROW_ID_FIELD_NAME)
		.arg(row_id), rdata))
My mistake – replacing argument cannot be used multiple times, so arguments need to be repeated: ``` if (row_id<=0 || true!=querySingleRecord( TQString("SELECT %1.%2 FROM %3 WHERE %4=%5").arg(tableName).arg(aiFieldName).arg(tableName) .arg(m_driver->beh->ROW_ID_FIELD_NAME == "ctid" ? aiFieldName : m_driver->beh->ROW_ID_FIELD_NAME) .arg(row_id), rdata)) ```
SlavekB marked this conversation as resolved
{
d->pqxxsql = new pqxx::connection( conninfo.latin1() );
drv_executeSQL( "SET DEFAULT_WITH_OIDS TO ON" ); //Postgres 8.1 changed the default to no oids but we need them
//drv_executeSQL( "SET DEFAULT_WITH_OIDS TO ON" ); //Postgres 8.1 changed the default to no oids but we need them
Owner

Probably nothing prevents the line from being removed instead of commenting.

Probably nothing prevents the line from being removed instead of commenting.
SlavekB marked this conversation as resolved
std::string temp;
TQString sql = "SELECT CURRVAL(pg_get_serial_sequence('";
sql += tableName + TQString::fromLatin1("', '");
sql += aiFieldName + TQString::fromLatin1("'))");
Owner

A possible better readable code?

TQString sql = TQString("SELECT CURRVAL(pg_get_serial_sequence('%1', '%2'))")
		.arg(tableName).arg(aiFieldName);
A possible better readable code? ``` TQString sql = TQString("SELECT CURRVAL(pg_get_serial_sequence('%1', '%2'))") .arg(tableName).arg(aiFieldName); ```
SlavekB marked this conversation as resolved
ormorph force-pushed feat/kexi-postgresql from 676b584d7f to becbce9bb5 5 months ago
ormorph changed title from Added kexi hotfix to support versions 12 and higher, fixing issue #15 to WIP: Added kexi hotfix to support versions 12 and higher, fixing issue #15 5 months ago
Poster
Collaborator

Added changes. It is still under development. It is necessary to improve the work with tables without a primary key. So far, there are problems with editing and deleting added rows in tables without a primary key.

Added changes. It is still under development. It is necessary to improve the work with tables without a primary key. So far, there are problems with editing and deleting added rows in tables without a primary key.
Poster
Collaborator

Hmm, there is another idea, to use xmin for indication, instead of ctid. I even made a working implementation. What do you think is best to use? I am attaching a patch of changes for xmin.
In any case, if you use ctid, you will also need to get the value in the request. The CURRVAL() function is not suitable for tables without a primary key. Therefore, it will be necessary to return the result of the INSERT INTO command in the same way, as it was done in the patch that I attached.
Using xmin requires fewer code changes.

Hmm, there is another idea, to use xmin for indication, instead of `ctid`. I even made a working implementation. What do you think is best to use? I am attaching a patch of changes for `xmin`. In any case, if you use ctid, you will also need to get the value in the request. The `CURRVAL()` function is not suitable for tables without a primary key. Therefore, it will be necessary to return the result of the `INSERT INTO` command in the same way, as it was done in the patch that I attached. Using `xmin` requires fewer code changes.
ormorph force-pushed feat/kexi-postgresql from becbce9bb5 to ec7f178f4a 5 months ago
Poster
Collaborator

Added changes, now tables without a primary key can be easily edited.
There are two variants:

  1. variant using ctid;
  2. variant using xmin (present as a patch).

Both options are working.

Added changes, now tables without a primary key can be easily edited. There are two variants: 1. variant using ctid; 2. variant using xmin (present as a patch). Both options are working.
SlavekB reviewed 5 months ago
SlavekB left a comment
Owner

I'm not an expert about PostgreSQL and therefore I can't thoroughly judge which variant is better. On the variant using xmin I like it is easier.

There is one small comment valid for both variants – see below.

I'm not an expert about PostgreSQL and therefore I can't thoroughly judge which variant is better. On the variant using `xmin` I like it is easier. There is one small comment valid for both variants – see below.
public:
TQString lastRowId;
Owner

I believe that there is no reason that the lastRowId variable was public – on the contrary, I think it should be private. This applies to both variants of patch – ctid as well as xmin.

I believe that there is no reason that the `lastRowId` variable was `public` – on the contrary, I think it should be `private`. This applies to both variants of patch – `ctid` as well as `xmin`.
SlavekB marked this conversation as resolved
ormorph force-pushed feat/kexi-postgresql from ec7f178f4a to 7115e4c672 5 months ago
Poster
Collaborator

I guess it's better to use xmin all the same, since it doesn't change if someone executes VACUUM FULL. I think that I will add a check to the xmin patch to determine the postgresql version and select the oid column for older versions.

Both ctid and xmin have 1 minus, when the rows are updated, the values of xmin and ctid change. Added and deleted lines are not affected, but modified ones are affected. Therefore, for tables that do not have a primary key, after changing the rows, you must close and reopen the tab with the table. But since the order is not important for rows that do not have a primary key, you can simply delete unnecessary rows and add new ones, then there is no need to update the table.

For history, I added patch files, for both implementations.

I guess it's better to use xmin all the same, since it doesn't change if someone executes VACUUM FULL. I think that I will add a check to the xmin patch to determine the postgresql version and select the oid column for older versions. Both ctid and xmin have 1 minus, when the rows are updated, the values of xmin and ctid change. Added and deleted lines are not affected, but modified ones are affected. Therefore, for tables that do not have a primary key, after changing the rows, you must close and reopen the tab with the table. But since the order is not important for rows that do not have a primary key, you can simply delete unnecessary rows and add new ones, then there is no need to update the table. For history, I added patch files, for both implementations.
ormorph force-pushed feat/kexi-postgresql from 7115e4c672 to 5274e5d455 5 months ago
Poster
Collaborator

Added changes to support xmin columns. Fixed problem with editing tables that do not have a primary key. Now there is no need for the old binding to oid columns. You can test the performance.

Added changes to support `xmin` columns. Fixed problem with editing tables that do not have a primary key. Now there is no need for the old binding to `oid` columns. You can test the performance.
ormorph changed title from WIP: Added kexi hotfix to support versions 12 and higher, fixing issue #15 to Added kexi hotfix to support versions 12 and higher, fixing issue #15 5 months ago
SlavekB approved these changes 5 months ago
SlavekB left a comment
Owner

Now all looks good.

Now all looks good.
SlavekB merged commit 5274e5d455 into master 5 months ago
SlavekB deleted branch feat/kexi-postgresql 5 months ago
SlavekB added this to the R14.0.12 release milestone 5 months ago

Reviewers

SlavekB approved these changes 5 months ago
The pull request has been merged as 5274e5d455.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.