> > Hi, > > > > The commit 2f094e7ac691abc9d2fe0f4dcf0feac4a6ce1d9c delivers an excellent > > feature, and the file `rewriteGraphTable.c` was > introduced as part of > > this commit. > > I've noticed that this code file could benefit from some tidying up, so > > please review the diff file attached below. > > This diff only includes **basic cleanup** of `rewriteGraphTable.c` ?C it??s > > just a simple pass, > > and there may be other minor omissions that haven??t been addressed yet. > > Hi Man, > Thanks for the patch. I reviewed the cleanup you propose. Most of the > changes look good to me, except the changes related to pstate. We have > both patterns in the code, one which uses free_parsestate() explicitly > and another that doesn't. In this case, we don't set > p_target_relation, and we don't touch p_next_resno. So the only thing > that free_parsestate() does it to free pstate, which will taken care > of by the memory context management. So not needed. Any other reason > you want to call free_parsestate() explicitly? > > Attached patch has changes other that pstate related changes. > > I think, Peter may want to collect more such changes before applying them.
Hi, Thank you for your review. I understand your feedback ?C this was a result of my personal coding habit: I prefer to explicitly call `free_parsestate()`, and also want to respect the comment associated with `make_parsestate()` (shown below): ``` /* * make_parsestate * Allocate and initialize a new ParseState. * * Caller should eventually release the ParseState via free_parsestate(). */ ``` For this reason, I have made minor modifications to this section of the code. -- regards, Man Zeng
