Обсуждение: Error position support for ComputeIndexAttrs
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 [0]: https://git.postgresql.org/cgit/postgresql.git/commit/?id=1e5e4efd02b614908cae62d9452528462d307224 [1]: https://postgr.es/m/202512121327.f2zimsr6guso@alvherre.pgsql -- jian https://www.enterprisedb.com/
Вложения
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