On Tue, Mar 17, 2026 at 6:56 AM zengman <[email protected]> wrote: > > > > 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` – 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 – 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): > ```
As Robert Haas explained in a nearby thread, we usually don't accept patches that modify code for personal preferences. We instead follow precedence. Also the email proposing the patches should explain why those changes are being proposed, especially when they are more than trivial changes. > /* > * 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. > Given that we have many other callers who do not call free_parsestate(), probably this should change, as a separate patch though. Maybe there is a pattern of when to call free_parsestate() and when not to. It will be good to add a clarifying comment here. -- Best Wishes, Ashutosh Bapat
