Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order some regression tests for stability on big-endian #1503

Closed
wants to merge 1 commit into from

Conversation

df7cb
Copy link
Contributor

@df7cb df7cb commented Jan 11, 2024

On Debian's s390x architecture, some regression tests were failing because the result set was reordered. Fix by attaching ORDER BY in problematic cases.

On top of this change, another fix is required: agtype_hash_cmp() changes values on big-endian architectures. For Debian, I fixed it by adding a regress/expected/agtype_1.out alternate output file where the diff to the original output file is this:

 diff -u regress/expected/agtype.out regress/expected/agtype_1.out
--- regress/expected/agtype.out	2024-01-11 13:03:54.051579131 +0100
+++ regress/expected/agtype_1.out	2024-01-11 14:33:37.961477838 +0100
@@ -3593,13 +3593,13 @@
 SELECT agtype_hash_cmp('1.0'::agtype);
  agtype_hash_cmp 
 -----------------
-       614780178
+      1437092844
 (1 row)
 
 SELECT agtype_hash_cmp('"1"'::agtype);
  agtype_hash_cmp 
 -----------------
-      -888576106
+     -1434266898
 (1 row)
 
 SELECT agtype_hash_cmp('[1]'::agtype);
@@ -3659,7 +3659,7 @@
 SELECT agtype_hash_cmp('[1, "abcde", 2.0]'::agtype);
  agtype_hash_cmp 
 -----------------
-     -1128310748
+       826120111
 (1 row)
 
 SELECT agtype_hash_cmp(agtype_in('null'));
@@ -3701,7 +3701,7 @@
 SELECT agtype_hash_cmp('{"id":1, "label":"test", "properties":{"id":100}}'::agtype);
  agtype_hash_cmp 
 -----------------
-      1116453668
+      -947461933
 (1 row)
 
 SELECT agtype_hash_cmp('{"id":1, "label":"test", "properties":{"id":100}}::vertex'::agtype);
@@ -3713,7 +3713,7 @@
 SELECT agtype_hash_cmp('{"id":2, "start_id":1, "end_id": 3, "label":"elabel", "properties":{}}'::agtype);
  agtype_hash_cmp 
 -----------------
-      1064722414
+      1662709842
 (1 row)
 
 SELECT agtype_hash_cmp('{"id":2, "start_id":1, "end_id": 3, "label":"elabel", "properties":{}}::edge'::agtype);

With that extra file, AGE passes all regression tests on apt.postgresql.org:
https://pgdgbuild.dus.dg-i.net/job/postgresql-16-age-binaries/

But maintaining a full agtype.out file along with a full agtype_1.out will be painful, so some other solution will be needed.

  • Move the 5 agtype_hash_cmp tests to a separate test file, so only these tests would need two files
  • Rewrite the test such that the different value isn't visible (perhaps check for <> 0 or similar)
  • Just don't test the output value
  • Rewrite agtype_hash_cmp to output the same value on all architectures

I can help implementing this, but I can't really decide which one of these options fits best. (The least intrusive version would probably option 1.)

On Debian's s390x architecture, some regression tests were failing
because the result set was reordered. Fix by attaching ORDER BY in
problematic cases.
@rafsun42
Copy link
Member

@df7cb Thank you for the suggestion. I lean towards the option 1: moving all agtype_hash_cmp tests to another file and creating two different expected files for big and little-endian systems. Postgresql takes a similar approach for their checksum tests.

If you have already worked on it, feel free to add the changes to this PR.

@jrgemignani
Copy link
Contributor

@df7cb @rafsun42 Can I get a status on this PR? Are we waiting on something or?

Copy link

This PR is stale because it has been open 45 days with no activity. Remove "Abondoned" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale Stale issues/PRs label May 11, 2024
Copy link

This PR was closed because it has been stalled for further 7 days with no activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master Stale Stale issues/PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants