TDE
/
koffice
7
0
Fork 0
Code Issues Pull Requests 1 Releases Wiki Activity
Labels Milestones

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

Merged
SlavekB merged 1 commits from feat/kexi-postgresql into master 2 years ago
Conversation 5 Commits 1 Files Changed 4
ormorph commented 2 years 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.
SlavekB reviewed 2 years 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.
kexi/kexidb/connection.cpp Outdated
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))
SlavekB commented 2 years ago
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)) ```
ormorph commented 2 years ago
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.
SlavekB commented 2 years ago
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
kexi/kexidb/drivers/pqxx/pqxxconnection.cpp Outdated
{
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
SlavekB commented 2 years ago
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
kexi/kexidb/drivers/pqxx/pqxxconnection.cpp Outdated
std::string temp;
TQString sql = "SELECT CURRVAL(pg_get_serial_sequence('";
sql += tableName + TQString::fromLatin1("', '");
sql += aiFieldName + TQString::fromLatin1("'))");
SlavekB commented 2 years ago
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 2 years 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 2 years ago
ormorph commented 2 years 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.
ormorph commented 2 years ago
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.
kexi-postgresql5.patch.gz
1.2 KiB
ormorph force-pushed feat/kexi-postgresql from becbce9bb5 to ec7f178f4a 2 years ago
ormorph commented 2 years 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 2 years 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.
kexi/kexidb/drivers/pqxx/pqxxconnection.h Outdated
public:
TQString lastRowId;
SlavekB commented 2 years ago
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 2 years ago
ormorph commented 2 years 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.
kexi-postgresql-ctid.patch.gz
2.5 KiB
kexi-postgresql-xmin.patch.gz
1.3 KiB
ormorph force-pushed feat/kexi-postgresql from 7115e4c672 to 5274e5d455 2 years ago
ormorph commented 2 years 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 2 years ago
SlavekB approved these changes 2 years ago
SlavekB left a comment
Owner

Now all looks good.

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

Reviewers

SlavekB approved these changes 2 years ago
The pull request has been merged as 5274e5d455.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
SlavekB
Labels
Apply labels
Clear labels
GE/need-info
General - need additional info from contributor PR/keep-branch
Pull request - do not delete branch after merging PR/not-ok
Pull request - need fixing PR/rfc
Pull request - request for comments PR/update-trans
Pull request - update to translation files needed PR/wip
Pull request - work in progress RS/R14.0.x
Related to R14.0.x series RS/R14.1.x
Related to R14.1.x series SL/critical
Severity level - critical SL/major
Severity level - major SL/minor
Severity level - minor SL/normal
Severity level - normal SL/regression
Severity level - regression from previous version SL/trivial
Severity level - trivial SL/wishlist
Severity level - wishlist request ST/duplicate
Status - duplicate of another issue ST/invalid
Status - invalid report ST/notourproblem
Status - not our problem ST/rejected
Status - rejected ST/wontfix
Status - won't fix ST/worksforme
Status - works for me, unable to reproduce
No Label GE/need-info PR/keep-branch PR/not-ok PR/rfc PR/update-trans PR/wip RS/R14.0.x RS/R14.1.x SL/critical SL/major SL/minor SL/normal SL/regression SL/trivial SL/wishlist ST/duplicate ST/invalid ST/notourproblem ST/rejected ST/wontfix ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.0.12 release
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/koffice#16
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'feat/kexi-postgresql'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes