Error position support for ComputeIndexAttrs

On Tue, Dec 16, 2025 at 12:51 PM jian he <jian.universality@gmail.com> wrote:

hi.

Following the addition of error position support to ComputePartitionAttrs in
[0], we can extend this feature to ComputeIndexAttrs.

Both partition keys and indexes support expressions and share a 32-column
limit, CREATE INDEX can be as complicated as PARTITION BY expression, and given
that ComputeIndexAttrs already contains 14 calls to ereport(ERROR, ...).
Adding error position support for ComputeIndexAttrs seems to make sense.

To achieve this, ComputeIndexAttrs must receive a ParseState. Since
ComputeIndexAttrs is nested under DefineIndex , DefineIndex must also have a
ParseState.

v1-0001: almost the same as [1], the only difference is after
makeNode(IndexElem),
we should set the location to -1.
v1-0002: Error position support for ComputeIndexAttrs

+1, patch looks quite straightforward and pretty much reasonable to me.

Regards,
Amul

On Tue, Dec 16, 2025 at 9:01 PM Amul Sul <sulamul@gmail.com> 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/regression.diffs
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

On Wed, 31 Dec 2025 at 13:14, jian he <jian.universality@gmail.com> wrote:

On Tue, Dec 16, 2025 at 9:01 PM Amul Sul <sulamul@gmail.com> 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/regression.diffs
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

On Wed, Dec 31, 2025 at 10:37 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

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.

hi.

This will not work for ``ALTER TABLE t ADD CONSTRAINT c UNIQUE (ii);``.
In this case, gram.y produces a single Constraint node, and Constraint node
contain only one location field. However, a unique location is required for each
IndexElem node.
Simply assigning the same location value to all IndexElem nodes does not seem
worth the effort required to add support for this.

see transformIndexConstraint:
``
foreach(lc, constraint->keys)
{
/* OK, add it to the index definition */
iparam = makeNode(IndexElem);
........
iparam->location = -1;
index->indexParams = lappend(index->indexParams, iparam);
}
``

also
``ALTER TABLE t ADD CONSTRAINT c UNIQUE (ii);``
the Constraint.location is the location of the word "CONSTRAINT",
which is far from the IndexElem location we want to report.

--
jian
https://www.enterprisedb.com

jian he <jian.universality@gmail.com> writes:

This will not work for ``ALTER TABLE t ADD CONSTRAINT c UNIQUE (ii);``.
In this case, gram.y produces a single Constraint node, and Constraint node
contain only one location field. However, a unique location is required for each
IndexElem node.

Yeah, the actual problem is that the column name(s) in Constraint are
just a list of String nodes without per-name locations.
transformIndexConstraint throws some errors using constraint->location
that really ought to point at an individual column name, so there's
room for improvement there, as seen in this example from the
regression tests:

-- UNIQUE with a range column/PERIOD that isn't there:
CREATE TABLE temporal_rng3 (
id INTEGER,
CONSTRAINT temporal_rng3_uq UNIQUE (id, valid_at WITHOUT OVERLAPS)
);
ERROR: column "valid_at" named in key does not exist
LINE 3: CONSTRAINT temporal_rng3_uq UNIQUE (id, valid_at WITHOUT O...
^

But this seems like material for a separate patch.

regards, tom lane

jian he <jian.universality@gmail.com> writes:

The attached patch only adjusts collate.linux.utf8.out and
collate.windows.win1252.out, with no other changes.

Pushed with minor adjustments -- mostly, I used indexelem->location
directly instead of going through exprLocation().

regards, tom lane