search

adamchainz / django-perf-rec

Keep detailed records of the performance of your Django code.
MIT License
349 stars 25 forks source link

Compatibility Issue with String Escapes in PostgreSQL after updating to psycopg3 #616

Open hsmett opened 5 months ago

hsmett commented 5 months ago

Python Version

3.9.10

Django Version

4.2.13

Package Version

4.25.0

Description

Hello,

I am encountering difficulties with django-perf-rec==4.25.0 after updating a project to django==4.2.13 and specifically migrating the library from psycopg2-binary==2.9.9 to psycopg[binary]==3.1.19.

The tests for this project need to be executed under PostgreSQL, and after the updates, some records differ:

- db: INSERT INTO "mytable" ("entity_id", "entity_ctype_id", "entity_owner_id", "username", "date", "type", "value") VALUES (#, #, #, #, #::timestamptz, #, #) RETURNING "mytable"."id"
+ db: INSERT INTO "mytable" ("entity_id", "entity_ctype_id", "entity_owner_id", "username", "date", "type", "value") VALUES (#, #, #, #, #::timestamptz, #, E#) RETURNING "mytable"."id"

Here, the value column is a TextField, and we are providing it with JSON text.

The prefix E seems to come from this PostgreSQL feature: String Constants with C-Style Escapes. Support for this feature appears to have been introduced by psycopg3, but it does not seem to be handled by django-perf-rec.

Since the addition of this prefix is conditional on its content, it seems reasonable that it should be excluded from the records. Could you consider updating the library to address this?

I am available if you need more information.

Thank you in advance.

adamchainz commented 5 months ago

At the moment, I am maintaining this project to keep it working on new versions, but not actively developing it. If you want to strip those prefixes, a PR with changelog note and tests would be welcome. The file to change is sql.py: https://github.com/adamchainz/django-perf-rec/blob/main/src/django_perf_rec/sql.py .

adamchainz commented 2 weeks ago

Okay, I've written #616 to fix this, but I'm not 100% on it. Due to sqlparse's tokenization, it's a lot of extra code to add.

Since the addition of this prefix is conditional on its content,

Do you mean that E-strings are only used if the string contains certain characters? Can you give an example?

hsmett commented 2 weeks ago

Thank you for the update

Do you mean that E-strings are only used if the string contains certain characters? Can you give an example?

That's right, here is an example: https://github.com/hsmett/django-perf-rec-issue-616/blob/main/issue/issue616/tests.py https://github.com/hsmett/django-perf-rec-issue-616/blob/main/issue/issue616/tests.perf.yml

Hugo

adamchainz commented 2 weeks ago

Hmm, right. I think I will wait to see if anyone else supports this issue first, and maybe someone has more insight.

There's also the possibility that sqlparse needs fixing first.

Or perhaps we could switch SQL parsing library. I saw this Mastodon post recently covering much faster alternatives.