Repository navigation
Fix MERGE edges on 32-bit builds: keep path_entry ids as graphids - #2602
Open
laquereric wants to merge 1 commit into
Open
laquereric wants to merge 1 commit into
laquereric wants to merge 1 commit into
Conversation
path_entry's id, start_id and end_id are graphid fields, but merge_vertex and merge_edge stored Datums in them and read them back as Datums, while prebuild_path stores real graphids for vertices bound by an earlier clause. On a 64-bit build the two are the same bits. With SIZEOF_DATUM=4 (the 32-bit support added in apache#2286, used by PGlite's wasm32 build) a graphid Datum is a pointer, so a MERGE of an edge onto bound vertices -- for example MERGE (a:N {k:'x'}) MERGE (b:N {k:'y'}) MERGE (a)-[:E]->(b) MATCH (a:N {k:'x'}), (b:N {k:'y'}) MERGE (a)-[:E]->(b) wrote the edge with start_id and end_id read from whatever the graphid, taken as an address, pointed to (0 in practice): the row is stored but no MATCH walks it, and a second MERGE of the same edge stores a duplicate. Convert with GRAPHID_GET_DATUM / DATUM_GET_GRAPHID where path_entry meets a Datum, guarding a NULL id. On 64-bit builds these are identity casts, so behaviour there is unchanged. Found running the same Cypher workload on PostgreSQL 17 (64-bit) and in PGlite 0.4.6 (wasm32): 606 of 617 statements agreed before, 617 after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2601.
path_entry'sid,start_idandend_idhold graphid values;merge_vertexandmerge_edgestored Datums in them and read them back as Datums, whileprebuild_pathstores real graphids for vertices bound by an earlier clause. With SIZEOF_DATUM = 4 (#2286) a graphid Datum is a pointer, so MERGE of an edge onto bound vertices wrote start_id = end_id = 0 (the edge is stored but never matched, and a second MERGE duplicates it).This converts with
GRAPHID_GET_DATUM/DATUM_GET_GRAPHIDwhereverpath_entrymeets a Datum, guarding a NULL id, and documents the invariant on the struct. On 64-bit builds these are identity casts, so behaviour there is unchanged.Tested:
make installcheck30 of 31; the one failure,age_load, is the environment (files outside/tmp/age/) and fails the same way without the change.A regression test cannot show the bug on the 64-bit CI; happy to add one for the 32-bit path if there is a way to run it.