Обсуждение: pgsql: Refactor ChangeVarNodesExtended() using the custom callback
Refactor ChangeVarNodesExtended() using the custom callback fc069a3a6319 implemented Self-Join Elimination (SJE) and put related logic to ChangeVarNodes_walker(). This commit provides refactoring to remove the SJE-related logic from ChangeVarNodes_walker() but adds a custom callback to ChangeVarNodesExtended(), which has a chance to process a node before ChangeVarNodes_walker(). Passing this callback to ChangeVarNodesExtended() allows SJE-related node handling to be kept within the analyzejoins.c. Reported-by: Richard Guo <guofenglinux@gmail.com> Discussion: https://postgr.es/m/CAMbWs49PE3CvnV8vrQ0Dr%3DHqgZZmX0tdNbzVNJxqc8yg-8kDQQ%40mail.gmail.com Author: Andrei Lepikhov <lepihov@gmail.com> Author: Alexander Korotkov <aekorotkov@gmail.com> Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/ab42d643c14509cf1345588f55d798284b11a91e Modified Files -------------- src/backend/optimizer/plan/analyzejoins.c | 160 +++++++++++++++++++++++++++--- src/backend/rewrite/rewriteManip.c | 138 ++++++-------------------- src/include/rewrite/rewriteManip.h | 17 +++- src/tools/pgindent/typedefs.list | 1 + 4 files changed, 192 insertions(+), 124 deletions(-)
Alexander, > Refactor ChangeVarNodesExtended() using the custom callback > > fc069a3a6319 implemented Self-Join Elimination (SJE) and put related logic > to ChangeVarNodes_walker(). This commit provides refactoring to remove the > SJE-related logic from ChangeVarNodes_walker() but adds a custom callback to > ChangeVarNodesExtended(), which has a chance to process a node before > ChangeVarNodes_walker(). Passing this callback to ChangeVarNodesExtended() > allows SJE-related node handling to be kept within the analyzejoins.c. I believe this was pushed by mistake. -- Best regards, Aleksander Alekseev
On Wed, May 7, 2025 at 11:47 AM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Refactor ChangeVarNodesExtended() using the custom callback > > > > fc069a3a6319 implemented Self-Join Elimination (SJE) and put related logic > > to ChangeVarNodes_walker(). This commit provides refactoring to remove the > > SJE-related logic from ChangeVarNodes_walker() but adds a custom callback to > > ChangeVarNodesExtended(), which has a chance to process a node before > > ChangeVarNodes_walker(). Passing this callback to ChangeVarNodesExtended() > > allows SJE-related node handling to be kept within the analyzejoins.c. > > I believe this was pushed by mistake. Why it should be mistake this time? At least, this time I managed to wait till the end of release freeze. ------ Regards, Alexander Korotkov Supabase
Hi, > > I believe this was pushed by mistake. > > Why it should be mistake this time? > At least, this time I managed to wait till the end of release freeze. Perhaps I misunderstand something in the process. How one will tag REL_18_BETA2 or fork REL_18_STABLE considering that the `master` branch contains this commit or others? -- Best regards, Aleksander Alekseev
Hi, Aleksander! On Wed, May 7, 2025 at 11:59 AM Aleksander Alekseev <aleksander@timescale.com> wrote: > > > I believe this was pushed by mistake. > > > > Why it should be mistake this time? > > At least, this time I managed to wait till the end of release freeze. > > Perhaps I misunderstand something in the process. How one will tag > REL_18_BETA2 or fork REL_18_STABLE considering that the `master` > branch contains this commit or others? How can this or other commits on master branch prevent creating new tag REL_18_BETA2 or forking REL_18_STABLE from master? Perhaps, this commit is not the only one on master branch after tag REL_18_BETA1. ------ Regards, Alexander Korotkov Supabase
Hi, > How can this or other commits on master branch prevent creating new > tag REL_18_BETA2 or forking REL_18_STABLE from master? Perhaps, this > commit is not the only one on master branch after tag REL_18_BETA1. Right, but I see people fixing whitespaces and documentation for PG18 in the `master` branch right now. My humble understanding is that `master` is basically REL_18_STABLE at the moment. If one wants to fork REL_18_STABLE for instance, either the branch will include your change (which we probably don't want), or one will have to manually revert it, or create a fork from BETA1 and cherry-pick all the necessary changes from `master`. This is the reason why I suspected this was committed by mistake. -- Best regards, Aleksander Alekseev
On Wed, May 7, 2025 at 12:47 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > How can this or other commits on master branch prevent creating new > > tag REL_18_BETA2 or forking REL_18_STABLE from master? Perhaps, this > > commit is not the only one on master branch after tag REL_18_BETA1. > > Right, but I see people fixing whitespaces and documentation for PG18 > in the `master` branch right now. My humble understanding is that > `master` is basically REL_18_STABLE at the moment. > > If one wants to fork REL_18_STABLE for instance, either the branch > will include your change (which we probably don't want), or one will > have to manually revert it, or create a fork from BETA1 and > cherry-pick all the necessary changes from `master`. > > This is the reason why I suspected this was committed by mistake. OK, now I get your point: you think this should go to the next major release. But no, it's not. It's intended to eventually land into REL_18_STABLE, REL_18_BETA2 etc. It addresses one of PG18 Open Items: Richard Guo found the way SJE changes ChangeVarNodes() unsatisfiable, and this commit is intended to fix that. Also, note that it is not the only PG18 Open Item left after beta1, and fixing Open Items usually requires changes besides docs and whitespaces. This commit changes ABI, but this should be OK. We do preserve ABI only for stable releases. ------ Regards, Alexander Korotkov Supabase
Hi, > OK, now I get your point: you think this should go to the next major > release. But no, it's not. It's intended to eventually land into > REL_18_STABLE, REL_18_BETA2 etc. It addresses one of PG18 Open Items: > Richard Guo found the way SJE changes ChangeVarNodes() unsatisfiable, > and this commit is intended to fix that. Also, note that it is not > the only PG18 Open Item left after beta1, and fixing Open Items > usually requires changes besides docs and whitespaces. Now I get it. From the commit message it looked like a regular refactoring. I didn't realize it fixes an open item. Many thanks! -- Best regards, Aleksander Alekseev