Use of sequences rather than autoincrement

[mdsupport]

.. it seems like it would be more robust to use a self incrementing id for most of the tables that use this rather than rely on the GenID function. Any folks know of the benefit of using this GenID mechanism over a simply id that counts for a table?

Originally posted by @bradymiller in https://github.com/openemr/openemr/issues/3788#issuecomment-669758698

Not sure if this strange legacy is noticed by anyone looking at database modernization. Databases are really good at keeping track of table row counters. Since encounters, documents and few other tables rely on this mechanism it creates random issues specially when transactions are not used.

Guess this should be part of #3273 as well.

[adunsulag]

I see it as a problem personally. It allows you to do inserts of dependent records without having to wait for the insert id of the previous record but I think that's actually an anti-pattern as everything should be inside of transactions anyways. I also saw a table last week that I can recall right now that specifically relied on the fact that the foreign keys were sequences to different tables and could be relied upon that way. Using the auto-increment would have resulted in duplicate foreign keys in the table. Again I see that as an anti-pattern and was super surprised to see it in the database as it makes it really hard to follow the relationships in the data. Perhaps other people see uses for this...

[mdsupport]

On a related note, the ORDataObject model uses REPLACE INTO taking away the option of hard error if two parallel processes got same ID because of lack of locks. So issue like #3788 is probably caused by a errant process that picked up existing ID while inserting a new document. Normally the INSERT error could have stopped the damage. But ORDataObject's persist removes all traces of old document while leaving the orphan physical file, the document category and pnote links untouched. Lot of finger pointing in the front office.

[adunsulag]

@mdsupport I didn't want to break backwards compatibility when I was modifying vitals by changing that REPLACE INTO. I was pretty tempted to change it though as when I was using foreign keys I noticed that REPLACE INTO does a delete record and then an insert. Maybe we should go ahead and change that in ORDataObject.

[mdsupport]

I just wanted to make future devs aware of these unexpected code variations since we assumed a library will implement add, change and delete methods. Combining "add+change" in single code is problematic behavior for such root class. I think ORDataObject comes through Smarty though it does not have any author block. So we find it easier to avoid its use for Add logic.

The reason I brought up this was that I am unsure how changing id to autoincrement will be handled by ORDataObject. It seemed like iterating through all columns using get/set methods for each field. So it could defeat/override the ddl change.

[mdsupport]

Possible fix to ORDataObject is :

1. Replace the current SHOW COLUMNS by native ADODB calls that return field objects for mySQL databases.
2. Where the sequences is called, check if any field (typically the primary key) is defined as autoincrement and skip call to get new sequence iff the field is empty. Guessing that the empty field should be dropped from list of columns as well in sql generation.
3. Examine the value of ORDataObject vs several useful ADODB helper methods rather than keep increasing the cost of maintenance and future development.

ORDataObject is used for many tables from really old code. In addition most sensitive use of sequences is in encounter + forms creation which happens with abundance throughout the code. We have no desire or resources to take on this task. So going to close the issue.

@adunsulag, on a related note re. foreign keys - be aware of the enormity of fixing existing data during the upgrade. Also spotty use of gprelations approach which relies on developers to do the work of db-engine will become redundant - a welcome progress.

[mdsupport]

@adunsulag, check #4862 that maintains backward compatibility in legacy code using ORDataObject.

Eliminating generate_id function will need to be handled on a per table basis. Easy targets are like insurance_companies table where id can be altered without any code change. On the other hand encounter related logic needs code change - also an opportunity to implement use of transactions to maintain integrity.

[mdsupport]

@sjpadgett Your #5365 about documents table is partially addressed in #4862. Global counter in a modern database is a strange thing to retain. Not only the current code single thread to do one extra update, it forces some weird code if you want to do any batch processing. Another sad part is ADODB does most of the things ORDataObject classes do in this application.

[mdsupport]

For anyone stumbling upon this post, categories_seq table is used like sequences table.

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity within the past 90 days. It will be closed in 7 days if no further activity occurs.

[stale[bot]]

This issue has been automatically closed because it has not had any recent activity within the past 97 days.