Patient Management in PostgreSQL

Tags: #<Tag:0x00007f01bbf1d1a0> #<Tag:0x00007f01bbf1cf98>

Hello everyone. I am currently running OpenMRS core on PostgreSQL with legacyUI and was going through the Patient Management section in Admin section. Everything works fine except when deleting a patient. When I am deleting the Patient forever I get a foreign key constraint error in table Patient.

ERROR: update or delete on table "person" violates foreign key constraint "person_id_for_patient" on table "patient"

Here is the link to the logs - https://pastebin.com/HF1P1Duu .

So I went through the database and saw that onDelete=CASCADE was not set in PostgreSQL for table patient. Then I crosschecked functionality with MySQL , there we can delete the patient even though it has onDelete=RESTRICT set.

My doubt is that there should have been an exception raised in MySQL as well, as is raised in PostgreSQL. How is that the Patient gets deleted successfully in MySQL even though onDelete=CASCADE is not set up? I went through the purge method but I couldn’t figure out the problem.

Well, I don’t know if making it impossible to delete a patient is necessarily a bug :grin:, but you should take a look at the SQL that’s being run. To do this, there’s a global property / setting called log.level to which you can add org.hibernate.SQL:debug to view the Hibernate-generated SQL (it’s very noisy… don’t do this often). When deleting a patient in a version of 2.2, I get the following SQL logs:

DEBUG - SqlStatementLogger.logStatement(109) |2020-04-30 13:39:02,255|
    delete
    from
        name_phonetics
    where
        person_name_id=?
DEBUG - SqlStatementLogger.logStatement(109) |2020-04-30 13:39:02,258|
    delete
    from
        person_address
    where
        person_address_id=?
DEBUG - SqlStatementLogger.logStatement(109) |2020-04-30 13:39:02,261|
    delete
    from
        person_name
    where
        person_name_id=?
DEBUG - SqlStatementLogger.logStatement(109) |2020-04-30 13:39:02,264|
    delete
    from
        patient_identifier
    where
        patient_identifier_id=?
DEBUG - SqlStatementLogger.logStatement(109) |2020-04-30 13:39:02,267|
    delete
    from
        patient
    where
        patient_id=?
DEBUG - SqlStatementLogger.logStatement(109) |2020-04-30 13:39:02,279|
    delete
    from
        person
    where
        person_id=? 

I.e., it’s properly deleting the patient before the person and so not touching the FK constraint. Is the generated SQL different in your setup?

1 Like

Thanks @ibacher , let me try this with PostgreSQL and see what’s the issue.

When deleting a patient forever with PostgreSQL, no query for deleting patient gets executed before deleting the person. Here are the SQL logs:

DEBUG - SqlStatementLogger.logStatement(128) |2020-05-05T13:05:04,389| 
    delete 
    from
        person_name 
    where
        person_name_id=?
DEBUG - SqlStatementLogger.logStatement(128) |2020-05-05T13:05:04,395| 
    delete 
    from
        patient_identifier 
    where
        patient_identifier_id=?
DEBUG - SqlStatementLogger.logStatement(128) |2020-05-05T13:05:04,396| 
    delete 
    from
        person 
    where
        person_id=?
ERROR - SqlExceptionHelper.logExceptions(142) |2020-05-05T13:05:04,410| Batch entry 0 delete from person where person_id=68 was aborted.  Call getNextException to see the cause.
ERROR - SqlExceptionHelper.logExceptions(142) |2020-05-05T13:05:04,411| ERROR: update or delete on table "person" violates foreign key constraint "person_id_for_patient" on table "patient"
  Detail: Key (person_id)=(68) is still referenced from table "patient".

Even though in hibernate configuration on-delete is set to cascade ( https://github.com/openmrs/openmrs-core/blob/4db76a7a8f66c3e9e94f776d64a609a686a12a2a/api/src/main/resources/org/openmrs/api/db/hibernate/Patient.hbm.xml#L20 ), still this is happening.

Any idea why this may be happening ?

@aman Thanks for this! This actually turned up something very valuable. By default, Hibernate depends on the database-level cascade. However, for some database dialects that don’t support this (like MySQL running with the MyISAM storage engine—which doesn’t even support foreign keys), it has a fallback: issuing multiple delete statements, as I showed when running against MySQL.

So the short answer to your question is that we need to ensure that the foreign key between Patient -> Person is setup with the ondelete property set to cascade. This should be addressed through a Liquibase changeset, which should be applied to (at a minimum) the patient table, the concept_numeric table, the concept_complex table, and the test_ordertable (these tables are all similarly joined sub-classes of other classes and so will have the same referential problem).

Now the more interesting part: OpenMRS actually goes out of it’s way to ensure that we are not using MyISAM as the storage engine. And the InnoDB storage engine supports foreign keys and even foreign keys with ondelete set to cascade. So why hasn’t this been an issue before? Well, Hibernate defaults to using the MyISAM dialect for MySQL databases. Consequently, we’ve been running InnoDB tables, but using the optimizations for MyISAM rather than InnoDB. This results in sub-optimal behaviour, like the multiple delete thing and Hibernate not disabling foreign key checks where appropriate (since MyISAM doesn’t support foreign keys). But it’s not broken.

This brings up a question, which I think is for the community as a whole: should we switch (in 2.4.0 and beyond only) to using the InnoDB dialect for Hibernate or leave things as they are? If so, we should create a changeset to address this not just for PostgreSQL, but for MySQL as well. If not, well I guess we’re ok. Thoughts?

@burke @dkayiwa @mseaton @mogoodrich @mksd

(Parenthetically, I’d also ask why DrugOrders, unlike TestOrders are not setup to cascade-delete when the parent order is deleted, but I suspect this is just an oversight because deleting is a very rare action).

1 Like

@ibacher I have added the onDelete = "CASCADE" in the createForeign key change set and the problem is resolved. I’ll keep in mind your points while moving forward. Thanks, for this. :slightly_smiling_face: