On Wed, Nov 15, 2023 at 5:09 PM Peter Eisentraut <pe...@eisentraut.org> wrote:
> On 14.11.23 11:40, Amul Sul wrote: > > Please have a look at the attached version, updating the syntax to have > "AS" > > after EXPRESSION and other changes suggested previously. > > The code structure looks good to me now. > Thank you for your review. > > Question: Why are you using AT_PASS_ADD_OTHERCONSTR? I don't know if > it's right or wrong, but if you have a specific reason, it would be good > to know. > I referred to ALTER COLUMN DEFAULT and used that. > I think ATExecSetExpression() needs to lock pg_attribute? Did you lose > that during the refactoring? > I have removed that intentionally since we were not updating anything in pg_attribute like ALTER DROP EXPRESSION. > > Tiny comment: The error message in ATExecSetExpression() does not need > to mention "stored", since it would be also applicable to virtual > generated columns in the future. > I had to have the same thought, but later decided when we do that virtual column thing, we could simply change that. I am fine to do that change now as well, let me know your thought. > Documentation additions in alter_table.sgml should use one-space indent > consistently. Also, "This form replaces expression" is missing a "the"? > Ok, will fix that. Regards, Amul
