Skip to content

Conversation

@markgomer
Copy link
Contributor

  • Initializing slot to NULL right at the declaration, which ensures that it has a valid value regardless of the execution path through the function.
  • Removing the multiple instances of estate->es_result_relation_info = saved_resultRelInfo; and completely eliminating saved_resultRelInfo. It seems this was not affecting any functionality in the code as the removal of es_result_relation_info restoration did not affect regression tests.
  • Refactoring the if and else conditions to avoid redundant code, which simplifies the function and makes it easier to read, with a clearer control flow.

Regression tests:

/usr/local/pgsql/lib/pgxs/src/makefiles/../../src/test/regress/pg_regress --inputdir=./ --bindir='/usr/local/pgsql/bin'    --load-extension=age --inputdir=.//regress --outputdir=.//regress --temp-instance=.//regress/instance --port=61958 --encoding=UTF-8 --dbname=contrib_regression scan graphid agtype catalog cypher expr cypher_create cypher_match cypher_unwind cypher_set cypher_remove cypher_delete cypher_with cypher_vle cypher_union cypher_call cypher_merge age_global_graph age_load index analyze graph_generation name_validation drop
============== creating temporary instance            ==============
============== initializing database system           ==============
============== starting postmaster                    ==============
running on port 61958 with PID 23622
============== creating database "contrib_regression" ==============
CREATE DATABASE
ALTER DATABASE
============== installing age                         ==============
CREATE EXTENSION
============== running regression test queries        ==============
test scan                         ... ok          250 ms
test graphid                      ... ok           16 ms
test agtype                       ... ok          168 ms
test catalog                      ... ok          156 ms
test cypher                       ... ok           28 ms
test expr                         ... ok          641 ms
test cypher_create                ... ok          144 ms
test cypher_match                 ... ok          563 ms
test cypher_unwind                ... ok           61 ms
test cypher_set                   ... ok          150 ms
test cypher_remove                ... ok           99 ms
test cypher_delete                ... ok          131 ms
test cypher_with                  ... ok          100 ms
test cypher_vle                   ... ok         1059 ms
test cypher_union                 ... ok           37 ms
test cypher_call                  ... ok           49 ms
test cypher_merge                 ... ok          221 ms
test age_global_graph             ... ok          187 ms
test age_load                     ... ok         1556 ms
test index                        ... ok          100 ms
test analyze                      ... ok           28 ms
test graph_generation             ... ok           99 ms
test name_validation              ... ok          146 ms
test drop                         ... ok          223 ms
============== shutting down postmaster               ==============
============== removing temporary instance            ==============

======================
 All 24 tests passed. 
======================

@jrgemignani
Copy link
Contributor

@markgomer Could you rebase/update this against the master branch? :)

@dehowef
Copy link
Member

dehowef commented Apr 19, 2024

@markgomer @jrgemignani any follow-up on this?

with clearer control flow and reduced redundancy

* In this commit, the exec_cypher_set function is substantially
refactored for improved clarity and efficiency.
* Initializations are made at the point of declaration, reducing
potential errors due to uninitialized variables.
* Control flow has been restructured to reduce nesting and
improve readability.
* Redundant code was identified and eliminated.
* The CommandCounterIncrement() call is now located at the
end of the function, ensuring its consistent execution.
* These changes result in a more readable, more maintainable
function with the same functionality.
@markgomer
Copy link
Contributor Author

markgomer commented Apr 19, 2024

Sorry for the delay, I've been out of it for a while. I have tested it in my own fork and it should be fine now. Please let me know

@jrgemignani
Copy link
Contributor

jrgemignani commented Apr 19, 2024

@dehowef @rafsun42 @MuhammadTahaNaveed @Zainab-Saad Can I get your thoughts on this?

@jrgemignani
Copy link
Contributor

@markgomer I am of the opinion that these specific changes are unnecessary and don't really make the code any more clear. That is not to say that I don't appreciate the effort. Additionally, the original author had poor coverage with regression tests so, I would be cautious about using them as a metric here.

@markgomer
Copy link
Contributor Author

@jrgemignani I agree to be honest, so I am closing this PR.
Back in the day I thought it would be ok if the regress tests passed, and they got a tiny boost in performance in my machine.
But I see your point about the changes. They don’t really add much and may break something unknown to me.

@markgomer markgomer closed this Apr 19, 2024
@markgomer markgomer deleted the tweaks branch June 14, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants