On Thu, Mar 27, 2025 at 10:08 AM Richard Guo <guofengli...@gmail.com> wrote: > On Thu, Mar 27, 2025 at 10:53 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Richard Guo <guofengli...@gmail.com> writes: > > > I'm planning to push this patch soon, barring any objections. > > > FWIW, I have not reviewed it at all. > > Oh, sorry. I'll hold off on pushing it.
As a general point, non-trivial patches should really get some substantive review before they are pushed. Please don't be in a rush to commit. It is very common for the time from when a patch is first posted to commit to be 3-6 months even for committers. Posting a brand-new feature patch on March 21st and then pressing to commit on March 27th is really not something you should be doing. I think it's particularly inappropriate here where you actually got a review that pointed out a serious design problem and then had to change the design. If you didn't get it right on the first try, you shouldn't be too confident that you did it perfectly the second time, either. I took a look at this today and I'm not entirely comfortable with this: + rel = table_open(rte->relid, NoLock); + + /* Record NOT NULL columns for this relation. */ + get_relation_notnullatts(rel, rte); + + table_close(rel, NoLock); As a general principle, I have found that it's usually a sign that something has been designed poorly when you find yourself wanting to open a relation, get exactly one piece of information, and close the relation again. That is why, today, all the information that the planner needs about a particular relation is retrieved by get_relation_info(). We do not just wander around doing random catalog lookups wherever we need some critical detail. This patch increases the number of places where we fetch relation data from 1 to 2, but it's still the case that almost everything happens in get_relation_info(), and there's now just exactly this 1 thing that is done in a different place. That doesn't seem especially nice. I thought the idea was going to be to move get_relation_info() to an earlier stage, not split one thing out of it while leaving everything else the same. -- Robert Haas EDB: http://www.enterprisedb.com
