On Fri, Jan 5, 2024 at 12:28 AM Peter Eisentraut <pe...@eisentraut.org> wrote:
> On 25.12.23 13:10, Amul Sul wrote: > > > I have committed this patch set. > > I couple of notes: > > You had included the moving of the AT_PASS_ADD_COL enum in the first > patch. This is not a good style. Refactoring patches should not > include semantic changes. I have moved that change the final patch that > introduced the new feature. > > I did not commit the 0002 patch that renamed some functions. I think > names like AlterColumn are too general, which makes this renaming > possibly counterproductive. I don't know a better name, maybe > AlterTypeOrSimilar, but that's obviously silly. I think leaving it at > AlterType isn't too bad, since most of the code is indeed for ALTER TYPE > support. We can reconsider this if we have a better idea. > > In RememberAllDependentForRebuilding(), I dropped some of the additional > errors that you introduced for the AT_SetExpression cases. These didn't > seem useful. For example, it is not possible for a generated column to > depend on another generated column, so there is no point checking for > it. Also, there were no test cases that covered any of these > situations. If we do want any of these, we should have tests and > documentation for them. > > For the tests that examine the EXPLAIN plans, I had to add an ANALYZE > after the SET EXPRESSION. Otherwise, I got unstable test results, > presumably because in some cases the analyze happened in the background. > > Understood. Thank you for your guidance and the commit. Regards, Amul
