On 23.11.23 15:13, Amul Sul wrote:
The exact sequencing of this seems to be tricky. It's clear that we
need to do it earlier than at the end. I also think it should be
strictly after AT_PASS_ALTER_TYPE so that the new expression can refer
to the new type of a column. It should also be after AT_PASS_ADD_COL,
so that the new expression can refer to any newly added column. But
then it's after AT_PASS_OLD_INDEX and AT_PASS_OLD_CONSTR, is that a
problem?
AT_PASS_ALTER_TYPE and AT_PASS_ADD_COL cannot be together, the ALTER TYPE
cannot see that column, I think we can adopt the samebehaviour.
With your v5 patch, I see the following behavior:
create table t1 (a int, b int generated always as (a + 1) stored);
alter table t1 add column c int, alter column b set expression as (a + c);
ERROR: 42703: column "c" does not exist
I think intuitively, this ought to work. Maybe just moving the new pass
after AT_PASS_ADD_COL would do it.
While looking at this, I figured that converting the AT_PASS_* macros to
an enum would make this code simpler and easier to follow. For patches
like yours you wouldn't have to renumber the whole list. We could put
this patch before yours if people agree with it.
I tried to reuse the code by borrowing code from ALTER TYPE, see if that
looks good to you.
But I have concerns, with that code reuse where we drop and re-add the
indexes
and constraints which seems unnecessary for SET EXPRESSION where column
attributes will stay the same. I don't know why ATLER TYPE does that for
index
since finish_heap_swap() anyway does reindexing. We could skip re-adding
index for SET EXPRESSION which would be fine but we could not skip the
re-addition of constraints, since rebuilding constraints for checking might
need a good amount of code copy especially for foreign key constraints.
Please have a look at the attached version, 0001 patch does the code
refactoring, and 0002 is the implementation, using the newly refactored
code to
re-add indexes and constraints for the validation. Added tests for the same.
This looks reasonable after a first reading. (I have found that using
git diff --patience makes the 0001 patch look more readable. Maybe
that's helpful.)From eac97b6f2a081f327d0649d35a346db0b4bb9d99 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 28 Nov 2023 12:04:03 +0100
Subject: [PATCH] Turn AT_PASS_* macros into an enum
---
src/backend/commands/tablecmds.c | 55 +++++++++++++++++---------------
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 30 insertions(+), 26 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 323d9bf870..7c675b834c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -142,20 +142,24 @@ static List *on_commits = NIL;
* a pass determined by subcommand type.
*/
-#define AT_PASS_UNSET -1 /* UNSET will cause ERROR */
-#define AT_PASS_DROP 0 /* DROP (all flavors) */
-#define AT_PASS_ALTER_TYPE 1 /* ALTER COLUMN TYPE */
-#define AT_PASS_OLD_INDEX 2 /* re-add existing indexes */
-#define AT_PASS_OLD_CONSTR 3 /* re-add existing constraints
*/
-/* We could support a RENAME COLUMN pass here, but not currently used */
-#define AT_PASS_ADD_COL 4 /* ADD COLUMN */
-#define AT_PASS_ADD_CONSTR 5 /* ADD constraints (initial
examination) */
-#define AT_PASS_COL_ATTRS 6 /* set column attributes, eg
NOT NULL */
-#define AT_PASS_ADD_INDEXCONSTR 7 /* ADD index-based constraints
*/
-#define AT_PASS_ADD_INDEX 8 /* ADD indexes */
-#define AT_PASS_ADD_OTHERCONSTR 9 /* ADD other constraints,
defaults */
-#define AT_PASS_MISC 10 /* other stuff */
-#define AT_NUM_PASSES 11
+typedef enum AlterTablePass
+{
+ AT_PASS_UNSET = -1, /* UNSET will cause ERROR */
+ AT_PASS_DROP, /* DROP (all flavors) */
+ AT_PASS_ALTER_TYPE, /* ALTER COLUMN TYPE */
+ AT_PASS_OLD_INDEX, /* re-add existing indexes */
+ AT_PASS_OLD_CONSTR, /* re-add existing constraints
*/
+ /* We could support a RENAME COLUMN pass here, but not currently used */
+ AT_PASS_ADD_COL, /* ADD COLUMN */
+ AT_PASS_ADD_CONSTR, /* ADD constraints (initial
examination) */
+ AT_PASS_COL_ATTRS, /* set column attributes, eg
NOT NULL */
+ AT_PASS_ADD_INDEXCONSTR, /* ADD index-based constraints */
+ AT_PASS_ADD_INDEX, /* ADD indexes */
+ AT_PASS_ADD_OTHERCONSTR, /* ADD other constraints, defaults */
+ AT_PASS_MISC, /* other stuff */
+} AlterTablePass;
+
+#define AT_NUM_PASSES (AT_PASS_MISC + 1)
typedef struct AlteredTableInfo
{
@@ -399,12 +403,12 @@ static void ATPrepCmd(List **wqueue, Relation rel,
AlterTableCmd *cmd,
static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context);
static void ATExecCmd(List **wqueue, AlteredTableInfo *tab,
- AlterTableCmd *cmd, LOCKMODE
lockmode, int cur_pass,
+ AlterTableCmd *cmd, LOCKMODE
lockmode, AlterTablePass cur_pass,
AlterTableUtilityContext *context);
static AlterTableCmd *ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab,
Relation rel, AlterTableCmd *cmd,
bool recurse, LOCKMODE lockmode,
-
int cur_pass,
+
AlterTablePass cur_pass,
AlterTableUtilityContext *context);
static void ATRewriteTables(AlterTableStmt *parsetree,
List **wqueue, LOCKMODE
lockmode,
@@ -427,7 +431,7 @@ static void ATPrepAddColumn(List **wqueue, Relation rel,
bool recurse, bool recu
static ObjectAddress ATExecAddColumn(List **wqueue, AlteredTableInfo *tab,
Relation rel, AlterTableCmd **cmd,
bool
recurse, bool recursing,
-
LOCKMODE lockmode, int cur_pass,
+
LOCKMODE lockmode, AlterTablePass cur_pass,
AlterTableUtilityContext *context);
static bool check_for_column_name_collision(Relation rel, const char *colname,
bool if_not_exists);
@@ -565,7 +569,7 @@ static void ATPostAlterTypeCleanup(List **wqueue,
AlteredTableInfo *tab,
static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
char *cmd,
List **wqueue, LOCKMODE lockmode,
bool rewrite);
-static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
+static void RebuildConstraintComment(AlteredTableInfo *tab, AlterTablePass
pass,
Oid
objid, Relation rel, List *domname,
const
char *conname);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
@@ -4742,7 +4746,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
AlterTableUtilityContext *context)
{
AlteredTableInfo *tab;
- int pass = AT_PASS_UNSET;
+ AlterTablePass pass;
/* Find or create work queue entry for this table */
tab = ATGetQueueEntry(wqueue, rel);
@@ -5116,7 +5120,6 @@ static void
ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
- int pass;
ListCell *ltab;
/*
@@ -5126,7 +5129,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
* re-adding of the foreign key constraint to the other table). Work
can
* only be propagated into later passes, however.
*/
- for (pass = 0; pass < AT_NUM_PASSES; pass++)
+ for (AlterTablePass pass = 0; pass <= AT_NUM_PASSES; pass++)
{
/* Go through each table that needs to be processed */
foreach(ltab, *wqueue)
@@ -5189,7 +5192,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
*/
static void
ATExecCmd(List **wqueue, AlteredTableInfo *tab,
- AlterTableCmd *cmd, LOCKMODE lockmode, int cur_pass,
+ AlterTableCmd *cmd, LOCKMODE lockmode, AlterTablePass
cur_pass,
AlterTableUtilityContext *context)
{
ObjectAddress address = InvalidObjectAddress;
@@ -5516,7 +5519,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
static AlterTableCmd *
ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
AlterTableCmd *cmd, bool recurse,
LOCKMODE lockmode,
- int cur_pass, AlterTableUtilityContext
*context)
+ AlterTablePass cur_pass,
AlterTableUtilityContext *context)
{
AlterTableCmd *newcmd = NULL;
AlterTableStmt *atstmt = makeNode(AlterTableStmt);
@@ -5554,7 +5557,7 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab,
Relation rel,
foreach(lc, atstmt->cmds)
{
AlterTableCmd *cmd2 = lfirst_node(AlterTableCmd, lc);
- int pass;
+ AlterTablePass pass;
/*
* This switch need only cover the subcommand types that can be
added
@@ -6959,7 +6962,7 @@ ATPrepAddColumn(List **wqueue, Relation rel, bool
recurse, bool recursing,
static ObjectAddress
ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
AlterTableCmd **cmd, bool recurse, bool
recursing,
- LOCKMODE lockmode, int cur_pass,
+ LOCKMODE lockmode, AlterTablePass cur_pass,
AlterTableUtilityContext *context)
{
Oid myrelid = RelationGetRelid(rel);
@@ -14235,7 +14238,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid
refRelId, char *cmd,
* entry; but callers already have them so might as well pass them.)
*/
static void
-RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
+RebuildConstraintComment(AlteredTableInfo *tab, AlterTablePass pass, Oid objid,
Relation rel, List *domname,
const char *conname)
{
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index dba3498a13..404a800fe4 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -97,6 +97,7 @@ AlterTSConfigurationStmt
AlterTSDictionaryStmt
AlterTableCmd
AlterTableMoveAllStmt
+AlterTablePass
AlterTableSpaceOptionsStmt
AlterTableStmt
AlterTableType
--
2.43.0
