age icon indicating copy to clipboard operation
age copied to clipboard

Simplify exec_cypher_set

Open markgomer opened this issue 1 year ago • 1 comments

  • 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. 
======================

markgomer avatar Jul 12 '23 13:07 markgomer

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

jrgemignani avatar Jan 02 '24 17:01 jrgemignani

@markgomer @jrgemignani any follow-up on this?

dehowef avatar Apr 19 '24 08:04 dehowef

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

markgomer avatar Apr 19 '24 13:04 markgomer

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

jrgemignani avatar Apr 19 '24 16:04 jrgemignani

@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.

jrgemignani avatar Apr 19 '24 16:04 jrgemignani

@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 avatar Apr 19 '24 17:04 markgomer