From 9b9c5f279e8261ab90dc64559911d2578288b7e9 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 21 Jan 2020 16:17:21 -0500 Subject: Clarify behavior of adding and altering a column in same ALTER command. The behavior of something like ALTER TABLE transactions ADD COLUMN status varchar(30) DEFAULT 'old', ALTER COLUMN status SET default 'current'; is to fill existing table rows with 'old', not 'current'. That's intentional and desirable for a couple of reasons: * It makes the behavior the same whether you merge the sub-commands into one ALTER command or give them separately; * If we applied the new default while filling the table, there would be no way to get the existing behavior in one SQL command. The same reasoning applies in cases that add a column and then manipulate its GENERATED/IDENTITY status in a second sub-command, since the generation expression is really just a kind of default. However, that wasn't very obvious (at least not to me; earlier in the referenced discussion thread I'd thought it was a bug to be fixed). And it certainly wasn't documented. Hence, add documentation, code comments, and a test case to clarify that this behavior is all intentional. In passing, adjust ATExecAddColumn's defaults-related relkind check so that it matches up exactly with ATRewriteTables, instead of being effectively (though not literally) the negated inverse condition. The reasoning can be explained a lot more concisely that way, too (not to mention that the comment now matches the code, which it did not before). Discussion: https://postgr.es/m/10365.1558909428@sss.pgh.pa.us --- src/backend/commands/tablecmds.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) (limited to 'src/backend/commands/tablecmds.c') diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 30b72b62971..faf0db99e2a 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -6126,14 +6126,18 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, * returned by AddRelationNewConstraints, so that the right thing happens * when a datatype's default applies. * - * We skip this step completely for views and foreign tables. For a view, - * we can only get here from CREATE OR REPLACE VIEW, which historically - * doesn't set up defaults, not even for domain-typed columns. And in any - * case we mustn't invoke Phase 3 on a view or foreign table, since they - * have no storage. - */ - if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE - && relkind != RELKIND_FOREIGN_TABLE && attribute.attnum > 0) + * Note: it might seem that this should happen at the end of Phase 2, so + * that the effects of subsequent subcommands can be taken into account. + * It's intentional that we do it now, though. The new column should be + * filled according to what is said in the ADD COLUMN subcommand, so that + * the effects are the same as if this subcommand had been run by itself + * and the later subcommands had been issued in new ALTER TABLE commands. + * + * We can skip this entirely for relations without storage, since Phase 3 + * is certainly not going to touch them. System attributes don't have + * interesting defaults, either. + */ + if (RELKIND_HAS_STORAGE(relkind) && attribute.attnum > 0) { /* * For an identity column, we can't use build_column_default(), -- cgit v1.2.3