Skip to content

Commit

Permalink
Get rid of recursion-marker values in enum AlterTableType
Browse files Browse the repository at this point in the history 
During ALTER TABLE execution, when prep-time handling of subcommands of
certain types determine that execution-time handling requires recursion,
they signal this by changing the subcommand type to a special value.
This can be done in a simpler way by using a separate flag introduced by
commit ec0925c, so do that.

Catversion bumped.  It's not clear to me that ALTER TABLE subcommands
are stored anywhere in catalogs (CREATE FUNCTION rejects it in BEGIN
ATOMIC function bodies), but we do have both write and read support for
them, so be safe.

Discussion: https://postgr.es/m/20220929090033.zxuaezcdwh2fgfjb@alvherre.pgsql
  • Loading branch information
@alvherre
alvherre committed Dec 12, 2022
1 parent 9d0cf57 commit 840ff5f
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 78 deletions.
67 changes: 13 additions & 54 deletions src/backend/commands/tablecmds.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4070,8 +4070,7 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
* For most subcommand types, phases 2 and 3 do no explicit recursion,
* since phase 1 already does it. However, for certain subcommand types
* it is only possible to determine how to recurse at phase 2 time; for
* those cases, phase 1 sets the cmd->recurse flag (or, in some older coding,
* changes the command subtype of a "Recurse" variant XXX to be cleaned up.)
* those cases, phase 1 sets the cmd->recurse flag.
*
* Thanks to the magic of MVCC, an error anywhere along the way rolls back
* the whole operation; we don't have to do anything special to clean up.
Expand Down Expand Up @@ -4276,7 +4275,6 @@ AlterTableGetLockLevel(List *cmds)
break;

case AT_AddConstraint:
case AT_AddConstraintRecurse: /* becomes AT_AddConstraint */
case AT_ReAddConstraint: /* becomes AT_AddConstraint */
case AT_ReAddDomainConstraint: /* becomes AT_AddConstraint */
if (IsA(cmd->def, Constraint))
Expand Down Expand Up @@ -4628,7 +4626,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
/* Recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
cmd->subtype = AT_AddConstraintRecurse;
cmd->recurse = true;
pass = AT_PASS_ADD_CONSTR;
break;
case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
Expand All @@ -4643,7 +4641,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
/* Other recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
cmd->subtype = AT_DropConstraintRecurse;
cmd->recurse = true;
pass = AT_PASS_DROP;
break;
case AT_AlterColumnType: /* ALTER COLUMN TYPE */
Expand Down Expand Up @@ -4765,7 +4763,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
/* Recursion occurs during execution phase */
/* No command-specific prep needed except saving recurse flag */
if (recurse)
cmd->subtype = AT_ValidateConstraintRecurse;
cmd->recurse = true;
pass = AT_PASS_MISC;
break;
case AT_ReplicaIdentity: /* REPLICA IDENTITY ... */
Expand Down Expand Up @@ -4930,12 +4928,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
case AT_AddColumn: /* ADD COLUMN */
case AT_AddColumnToView: /* add column via CREATE OR REPLACE VIEW */
address = ATExecAddColumn(wqueue, tab, rel, &cmd,
false, false,
lockmode, cur_pass, context);
break;
case AT_AddColumnRecurse:
address = ATExecAddColumn(wqueue, tab, rel, &cmd,
true, false,
cmd->recurse, false,
lockmode, cur_pass, context);
break;
case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */
Expand Down Expand Up @@ -4989,13 +4982,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
break;
case AT_DropColumn: /* DROP COLUMN */
address = ATExecDropColumn(wqueue, rel, cmd->name,
cmd->behavior, false, false,
cmd->missing_ok, lockmode,
NULL);
break;
case AT_DropColumnRecurse: /* DROP COLUMN with recursion */
address = ATExecDropColumn(wqueue, rel, cmd->name,
cmd->behavior, true, false,
cmd->behavior, cmd->recurse, false,
cmd->missing_ok, lockmode,
NULL);
break;
Expand All @@ -5015,27 +5002,14 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
/* Transform the command only during initial examination */
if (cur_pass == AT_PASS_ADD_CONSTR)
cmd = ATParseTransformCmd(wqueue, tab, rel, cmd,
false, lockmode,
cmd->recurse, lockmode,
cur_pass, context);
/* Depending on constraint type, might be no more work to do now */
if (cmd != NULL)
address =
ATExecAddConstraint(wqueue, tab, rel,
(Constraint *) cmd->def,
false, false, lockmode);
break;
case AT_AddConstraintRecurse: /* ADD CONSTRAINT with recursion */
/* Transform the command only during initial examination */
if (cur_pass == AT_PASS_ADD_CONSTR)
cmd = ATParseTransformCmd(wqueue, tab, rel, cmd,
true, lockmode,
cur_pass, context);
/* Depending on constraint type, might be no more work to do now */
if (cmd != NULL)
address =
ATExecAddConstraint(wqueue, tab, rel,
(Constraint *) cmd->def,
true, false, lockmode);
cmd->recurse, false, lockmode);
break;
case AT_ReAddConstraint: /* Re-add pre-existing check constraint */
address =
Expand All @@ -5060,22 +5034,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
address = ATExecAlterConstraint(rel, cmd, false, false, lockmode);
break;
case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
address = ATExecValidateConstraint(wqueue, rel, cmd->name, false,
false, lockmode);
break;
case AT_ValidateConstraintRecurse: /* VALIDATE CONSTRAINT with
* recursion */
address = ATExecValidateConstraint(wqueue, rel, cmd->name, true,
address = ATExecValidateConstraint(wqueue, rel, cmd->name, cmd->recurse,
false, lockmode);
break;
case AT_DropConstraint: /* DROP CONSTRAINT */
ATExecDropConstraint(rel, cmd->name, cmd->behavior,
false, false,
cmd->missing_ok, lockmode);
break;
case AT_DropConstraintRecurse: /* DROP CONSTRAINT with recursion */
ATExecDropConstraint(rel, cmd->name, cmd->behavior,
true, false,
cmd->recurse, false,
cmd->missing_ok, lockmode);
break;
case AT_AlterColumnType: /* ALTER COLUMN TYPE */
Expand Down Expand Up @@ -5351,7 +5315,7 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
case AT_AddConstraint:
/* Recursion occurs during execution phase */
if (recurse)
cmd2->subtype = AT_AddConstraintRecurse;
cmd2->recurse = true;
switch (castNode(Constraint, cmd2->def)->contype)
{
case CONSTR_PRIMARY:
Expand Down Expand Up @@ -6110,7 +6074,6 @@ alter_table_type_to_string(AlterTableType cmdtype)
switch (cmdtype)
{
case AT_AddColumn:
case AT_AddColumnRecurse:
case AT_AddColumnToView:
return "ADD COLUMN";
case AT_ColumnDefault:
Expand All @@ -6135,24 +6098,20 @@ alter_table_type_to_string(AlterTableType cmdtype)
case AT_SetCompression:
return "ALTER COLUMN ... SET COMPRESSION";
case AT_DropColumn:
case AT_DropColumnRecurse:
return "DROP COLUMN";
case AT_AddIndex:
case AT_ReAddIndex:
return NULL; /* not real grammar */
case AT_AddConstraint:
case AT_AddConstraintRecurse:
case AT_ReAddConstraint:
case AT_ReAddDomainConstraint:
case AT_AddIndexConstraint:
return "ADD CONSTRAINT";
case AT_AlterConstraint:
return "ALTER CONSTRAINT";
case AT_ValidateConstraint:
case AT_ValidateConstraintRecurse:
return "VALIDATE CONSTRAINT";
case AT_DropConstraint:
case AT_DropConstraintRecurse:
return "DROP CONSTRAINT";
case AT_ReAddComment:
return NULL; /* not real grammar */
Expand Down Expand Up @@ -6671,7 +6630,7 @@ ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recursing,
ATTypedTableRecursion(wqueue, rel, cmd, lockmode, context);

if (recurse && !is_view)
cmd->subtype = AT_AddColumnRecurse;
cmd->recurse = true;
}

/*
Expand Down Expand Up @@ -8376,7 +8335,7 @@ ATPrepDropColumn(List **wqueue, Relation rel, bool recurse, bool recursing,
ATTypedTableRecursion(wqueue, rel, cmd, lockmode, context);

if (recurse)
cmd->subtype = AT_DropColumnRecurse;
cmd->recurse = true;
}

/*
Expand Down
2 changes: 0 additions & 2 deletions src/backend/parser/parse_utilcmd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3362,7 +3362,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
switch (cmd->subtype)
{
case AT_AddColumn:
case AT_AddColumnRecurse:
{
ColumnDef *def = castNode(ColumnDef, cmd->def);

Expand All @@ -3386,7 +3385,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
}

case AT_AddConstraint:
case AT_AddConstraintRecurse:

/*
* The original AddConstraint cmd node doesn't go to newcmds
Expand Down
2 changes: 1 addition & 1 deletion src/include/catalog/catversion.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,6 @@
*/

/* yyyymmddN */
#define CATALOG_VERSION_NO 202212092
#define CATALOG_VERSION_NO 202212121

#endif
5 changes: 0 additions & 5 deletions src/include/nodes/parsenodes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1979,7 +1979,6 @@ typedef struct AlterTableStmt
typedef enum AlterTableType
{
AT_AddColumn, /* add column */
AT_AddColumnRecurse, /* internal to commands/tablecmds.c */
AT_AddColumnToView, /* implicitly via CREATE OR REPLACE VIEW */
AT_ColumnDefault, /* alter column default */
AT_CookedColumnDefault, /* add a pre-cooked column default */
Expand All @@ -1993,19 +1992,15 @@ typedef enum AlterTableType
AT_SetStorage, /* alter column set storage */
AT_SetCompression, /* alter column set compression */
AT_DropColumn, /* drop column */
AT_DropColumnRecurse, /* internal to commands/tablecmds.c */
AT_AddIndex, /* add index */
AT_ReAddIndex, /* internal to commands/tablecmds.c */
AT_AddConstraint, /* add constraint */
AT_AddConstraintRecurse, /* internal to commands/tablecmds.c */
AT_ReAddConstraint, /* internal to commands/tablecmds.c */
AT_ReAddDomainConstraint, /* internal to commands/tablecmds.c */
AT_AlterConstraint, /* alter constraint */
AT_ValidateConstraint, /* validate constraint */
AT_ValidateConstraintRecurse, /* internal to commands/tablecmds.c */
AT_AddIndexConstraint, /* add constraint using existing index */
AT_DropConstraint, /* drop constraint */
AT_DropConstraintRecurse, /* internal to commands/tablecmds.c */
AT_ReAddComment, /* internal to commands/tablecmds.c */
AT_AlterColumnType, /* alter column type */
AT_AlterColumnGenericOptions, /* alter column OPTIONS (...) */
Expand Down
20 changes: 4 additions & 16 deletions src/test/modules/test_ddl_deparse/test_ddl_deparse.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,6 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
case AT_AddColumn:
strtype = "ADD COLUMN";
break;
case AT_AddColumnRecurse:
strtype = "ADD COLUMN (and recurse)";
break;
case AT_AddColumnToView:
strtype = "ADD COLUMN TO VIEW";
break;
Expand Down Expand Up @@ -156,9 +153,6 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
case AT_DropColumn:
strtype = "DROP COLUMN";
break;
case AT_DropColumnRecurse:
strtype = "DROP COLUMN (and recurse)";
break;
case AT_AddIndex:
strtype = "ADD INDEX";
break;
Expand All @@ -168,9 +162,6 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
case AT_AddConstraint:
strtype = "ADD CONSTRAINT";
break;
case AT_AddConstraintRecurse:
strtype = "ADD CONSTRAINT (and recurse)";
break;
case AT_ReAddConstraint:
strtype = "(re) ADD CONSTRAINT";
break;
Expand All @@ -183,18 +174,12 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
case AT_ValidateConstraint:
strtype = "VALIDATE CONSTRAINT";
break;
case AT_ValidateConstraintRecurse:
strtype = "VALIDATE CONSTRAINT (and recurse)";
break;
case AT_AddIndexConstraint:
strtype = "ADD CONSTRAINT (using index)";
break;
case AT_DropConstraint:
strtype = "DROP CONSTRAINT";
break;
case AT_DropConstraintRecurse:
strtype = "DROP CONSTRAINT (and recurse)";
break;
case AT_ReAddComment:
strtype = "(re) ADD COMMENT";
break;
Expand Down Expand Up @@ -326,7 +311,10 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
break;
}

values[0] = CStringGetTextDatum(strtype);
if (subcmd->recurse)
values[0] = CStringGetTextDatum(psprintf("%s (and recurse)", strtype));
else
values[0] = CStringGetTextDatum(strtype);
if (OidIsValid(sub->address.objectId))
{
char *objdesc;
Expand Down

0 comments on commit 840ff5f

Please sign in to comment.