public inbox for [email protected]
help / color / mirror / Atom feedFrom: Kirill Reshke <[email protected]>
To: jian he <[email protected]>
Cc: Amul Sul <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Error position support for ComputeIndexAttrs
Date: Wed, 31 Dec 2025 19:37:13 +0500
Message-ID: <CALdSSPjm8dB6ypEkivNMQp48U6YRv0VofX7KndS2Nq0DZO+OKg@mail.gmail.com> (raw)
In-Reply-To: <CACJufxHQLuROz_1MjuvNSp+QgtLfNH_noiq18mZBhB8Mk01_=g@mail.gmail.com>
References: <CACJufxH3OgXF1hrzGAaWyNtye2jHEmk9JbtrtGv-KJK6tsGo5w@mail.gmail.com>
<CAAJ_b95FOL6uEUb6p4=ho7Bj=HFBpzhRh5G0G2--YH9qHxt0rw@mail.gmail.com>
<CACJufxHQLuROz_1MjuvNSp+QgtLfNH_noiq18mZBhB8Mk01_=g@mail.gmail.com>
On Wed, 31 Dec 2025 at 13:14, jian he <[email protected]> wrote:
>
> On Tue, Dec 16, 2025 at 9:01 PM Amul Sul <[email protected]> wrote:
> > >
> >
> > +1, patch looks quite straightforward and pretty much reasonable to me.
> >
> > Regards,
> > Amul
>
> hi.
>
> some failures because I didn't adjust collate.linux.utf8.out and
> collate.windows.win1252.out:
> https://cirrus-ci.com/build/6690791764000768
> https://api.cirrus-ci.com/v1/artifact/task/5658162642026496/testrun/build/testrun/regress/regress/re...
> https://api.cirrus-ci.com/v1/artifact/task/5605386083893248/log/src/test/regress/regression.diffs
>
> The attached patch only adjusts collate.linux.utf8.out and
> collate.windows.win1252.out, with no other changes.
>
>
> --
> jian
> https://www.enterprisedb.com
Hi!
Nice catch, I see this patch is indeed an improvement.
For v2-0002, I was wondering if there is any further improvements
possible. Namely, if we can pass parse state in cases where v-0002
passes NULL.
1) So, DefineIndex in bootstrap (src/backend/bootstrap/bootparse.y) -
obviously there is no need to support error position for bootstrap
2) DefineIndex in commands/indexcmds.c L1524 (inside DefineIndex
actually) - We do not need to pass parse state here, because if any
trouble, we will elog(ERROR) earlier.
3) DefineIndex in commands/tablecmds.c L 1305 - also executed in
child partition cases.
4) DefineIndex inside ATAddIndex. It is triggered for patterns like
"alter table t add constraint c unique (ii);" - current patch set
missing support for them. I tried to pass pstate here, but no success,
because exprLocation returns -1 in ComputeIndexAttrs. Please see my
attempt attached. I guess this can be completed to get location
support, but I do not have any time today.
5) DefineIndex inside AttachPartitionEnsureIndexes - looks like we
do not need pstate here.
--
Best regards,
Kirill Reshke
Attachments:
[application/octet-stream] patch.diff (8.0K, ../CALdSSPjm8dB6ypEkivNMQp48U6YRv0VofX7KndS2Nq0DZO+OKg@mail.gmail.com/2-patch.diff)
download | inline diff:
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ef564e9e3bf..c77ab07bc72 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1957,14 +1957,14 @@ ComputeIndexAttrs(ParseState *pstate,
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" named in key does not exist",
- attribute->name)),
- parser_errposition(pstate, exprLocation((Node *) attribute)));
+ attribute->name),
+ parser_errposition(pstate, exprLocation((Node *) attribute))));
else
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" does not exist",
- attribute->name)),
- parser_errposition(pstate, exprLocation((Node *) attribute)));
+ attribute->name),
+ parser_errposition(pstate, exprLocation((Node *) attribute))));
}
attform = (Form_pg_attribute) GETSTRUCT(atttuple);
indexInfo->ii_IndexAttrNumbers[attn] = attform->attnum;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 32be3d3ca75..5f58f594582 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -456,15 +456,15 @@ static void validateForeignKeyConstraint(char *conname,
Relation rel, Relation pkrel,
Oid pkindOid, Oid constraintOid, bool hasperiod);
static void CheckAlterTableIsSafe(Relation rel);
-static void ATController(AlterTableStmt *parsetree,
+static void ATController(ParseState *pstate, AlterTableStmt *parsetree,
Relation rel, List *cmds, bool recurse, LOCKMODE lockmode,
AlterTableUtilityContext *context);
static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
bool recurse, bool recursing, LOCKMODE lockmode,
AlterTableUtilityContext *context);
-static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
+static void ATRewriteCatalogs(ParseState *pstate, List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context);
-static void ATExecCmd(List **wqueue, AlteredTableInfo *tab,
+static void ATExecCmd(ParseState *pstate, List **wqueue, AlteredTableInfo *tab,
AlterTableCmd *cmd, LOCKMODE lockmode, AlterTablePass cur_pass,
AlterTableUtilityContext *context);
static AlterTableCmd *ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab,
@@ -542,7 +542,7 @@ static void ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
bool recurse, LOCKMODE lockmode,
AlterTableUtilityContext *context);
static void verifyNotNullPKCompatible(HeapTuple tuple, const char *colname);
-static ObjectAddress ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
+static ObjectAddress ATExecAddIndex(ParseState *pstate, AlteredTableInfo *tab, Relation rel,
IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode);
static ObjectAddress ATExecAddStatistics(AlteredTableInfo *tab, Relation rel,
CreateStatsStmt *stmt, bool is_rebuild, LOCKMODE lockmode);
@@ -4529,7 +4529,7 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
* Some of the fields therein, such as the relid, are used here as well.
*/
void
-AlterTable(AlterTableStmt *stmt, LOCKMODE lockmode,
+AlterTable(ParseState *pstate, AlterTableStmt *stmt, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
Relation rel;
@@ -4539,7 +4539,7 @@ AlterTable(AlterTableStmt *stmt, LOCKMODE lockmode,
CheckAlterTableIsSafe(rel);
- ATController(stmt, rel, stmt->cmds, stmt->relation->inh, lockmode, context);
+ ATController(pstate, stmt, rel, stmt->cmds, stmt->relation->inh, lockmode, context);
}
/*
@@ -4567,7 +4567,7 @@ AlterTableInternal(Oid relid, List *cmds, bool recurse)
EventTriggerAlterTableRelid(relid);
- ATController(NULL, rel, cmds, recurse, lockmode, NULL);
+ ATController(NULL, NULL, rel, cmds, recurse, lockmode, NULL);
}
/*
@@ -4870,7 +4870,7 @@ AlterTableGetLockLevel(List *cmds)
* when requested.
*/
static void
-ATController(AlterTableStmt *parsetree,
+ATController(ParseState *pstate, AlterTableStmt *parsetree,
Relation rel, List *cmds, bool recurse, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
@@ -4889,7 +4889,7 @@ ATController(AlterTableStmt *parsetree,
relation_close(rel, NoLock);
/* Phase 2: update system catalogs */
- ATRewriteCatalogs(&wqueue, lockmode, context);
+ ATRewriteCatalogs(pstate, &wqueue, lockmode, context);
/* Phase 3: scan/rewrite tables as needed, and run afterStmts */
ATRewriteTables(parsetree, &wqueue, lockmode, context);
@@ -5308,7 +5308,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
* conflicts).
*/
static void
-ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
+ATRewriteCatalogs(ParseState *pstate, List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
ListCell *ltab;
@@ -5340,7 +5340,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
tab->rel = relation_open(tab->relid, NoLock);
foreach(lcmd, subcmds)
- ATExecCmd(wqueue, tab,
+ ATExecCmd(pstate, wqueue, tab,
lfirst_node(AlterTableCmd, lcmd),
lockmode, pass, context);
@@ -5382,7 +5382,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
* ATExecCmd: dispatch a subcommand to appropriate execution routine
*/
static void
-ATExecCmd(List **wqueue, AlteredTableInfo *tab,
+ATExecCmd(ParseState *pstate, List **wqueue, AlteredTableInfo *tab,
AlterTableCmd *cmd, LOCKMODE lockmode, AlterTablePass cur_pass,
AlterTableUtilityContext *context)
{
@@ -5454,11 +5454,11 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
NULL);
break;
case AT_AddIndex: /* ADD INDEX */
- address = ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, false,
+ address = ATExecAddIndex(pstate, tab, rel, (IndexStmt *) cmd->def, false,
lockmode);
break;
case AT_ReAddIndex: /* ADD INDEX */
- address = ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, true,
+ address = ATExecAddIndex(pstate, tab, rel, (IndexStmt *) cmd->def, true,
lockmode);
break;
case AT_ReAddStatistics: /* ADD STATISTICS */
@@ -9646,7 +9646,7 @@ verifyNotNullPKCompatible(HeapTuple tuple, const char *colname)
* Return value is the address of the new index.
*/
static ObjectAddress
-ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
+ATExecAddIndex(ParseState *pstate, AlteredTableInfo *tab, Relation rel,
IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode)
{
bool check_rights;
@@ -9667,7 +9667,7 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
/* suppress notices when rebuilding existing index */
quiet = is_rebuild;
- address = DefineIndex(NULL,
+ address = DefineIndex(pstate,
RelationGetRelid(rel),
stmt,
InvalidOid, /* no predefined OID */
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index b611eab0550..1d07b1b0ce3 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1317,7 +1317,7 @@ ProcessUtilitySlow(ParseState *pstate,
EventTriggerAlterTableRelid(relid);
/* ... and do it */
- AlterTable(atstmt, lockmode, &atcontext);
+ AlterTable(pstate, atstmt, lockmode, &atcontext);
/* done */
EventTriggerAlterTableEnd();
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index e9b0fab0767..d0ff75e938f 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -17,6 +17,7 @@
#include "access/htup.h"
#include "catalog/dependency.h"
#include "catalog/objectaddress.h"
+#include "parser/parse_node.h"
#include "nodes/parsenodes.h"
#include "storage/lock.h"
#include "utils/relcache.h"
@@ -34,7 +35,7 @@ extern void RemoveRelations(DropStmt *drop);
extern Oid AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode);
-extern void AlterTable(AlterTableStmt *stmt, LOCKMODE lockmode,
+extern void AlterTable(ParseState *pstate, AlterTableStmt *stmt, LOCKMODE lockmode,
AlterTableUtilityContext *context);
extern LOCKMODE AlterTableGetLockLevel(List *cmds);
view thread (4+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Error position support for ComputeIndexAttrs
In-Reply-To: <CALdSSPjm8dB6ypEkivNMQp48U6YRv0VofX7KndS2Nq0DZO+OKg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox