Pass ParseState as down to utility functions.

hi.

ATExecAddOf
DefineType
ATPrepAlterColumnType
ATExecAlterColumnType
DefineDomain
AlterType
i changed the above function, so the above related function errors may
print out error position.
reason for change mainly because these functions have
`typenameType(NULL, typeName, &targettypmod);`
we want to pass not NULL pstate (typenameType(pstate, typeName, &targettypmod);)

why do we want printout error position
1. it can quickly locate DDL command error positions, beginner friendly.
2. in the thread `CREATE SCHEMA ... CREATE DOMAIN support`, case like:
CREATE SCHEMA regress_schema_2
create domain ss1 as ss
create domain ss as text;
ERROR: type "ss" does not exist
obviously the error is not helpful at all.

As you can see, in cases like a single DDL, multiple sub DDL within
it, error position is quite important
I also added some tests for DefineDomain.
added parser_errposition for many places in DefineDomain.

the attached patch (based on Kirill Reshke 's v2 patch)
either passing the source_string to the existing ParseState
or by making a new ParseState passing source_string to it.
then add
`parser_errposition(pstate, location)))`
in various places optionally.

So I guess bundling it into a single patch should be fine?

On Sat, 30 Nov 2024 at 17:37, jian he <jian.universality@gmail.com> wrote:

So I guess bundling it into a single patch should be fine?

Ok. I created CF entry for this patch.

[0]: https://commitfest.postgresql.org/51/5420/ -- Best regards, Kirill Reshke
--
Best regards,
Kirill Reshke

On Wed, Dec 04, 2024 at 03:31:47PM +0500, Kirill Reshke wrote:

On Sat, 30 Nov 2024 at 17:37, jian he <jian.universality@gmail.com> wrote:

So I guess bundling it into a single patch should be fine?

Ok. I created CF entry for this patch.

[0] https://commitfest.postgresql.org/51/5420/

Note that v3 of the patch is failing in the CI, so you should look at
that:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5420

Combining everything into a single patch is not a big deal in this
case IMO as the code paths touched are different.

I was playing with the patch and tried how typenameType() would like
to force a rule so as the pstate should be always non-NULL, and got
reminded by the various callers of typenameTypeIdAndMod() that this
was a bad idea.

This patch does not show how the error reports are influenced for
DefineType() and AlterType().

This reminds as well that there is little coverage for many error
paths of DefineDomain(), with some paths actually modified in this
patch:
https://coverage.postgresql.org/src/backend/commands/typecmds.c.gcov.html
I'd suggest to do something about that, while on it, to check that the
parser issues a location on error.
--
Michael

On Thu, 5 Dec 2024 at 11:45, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Dec 04, 2024 at 03:31:47PM +0500, Kirill Reshke wrote:

On Sat, 30 Nov 2024 at 17:37, jian he <jian.universality@gmail.com> wrote:

So I guess bundling it into a single patch should be fine?

Ok. I created CF entry for this patch.

[0] https://commitfest.postgresql.org/51/5420/

Note that v3 of the patch is failing in the CI, so you should look at
that:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5420

Indeed. Thank you.

Combining everything into a single patch is not a big deal in this
case IMO as the code paths touched are different.

Ok.

I was playing with the patch and tried how typenameType() would like
to force a rule so as the pstate should be always non-NULL, and got
reminded by the various callers of typenameTypeIdAndMod() that this
was a bad idea.

I'm not quite understand what you're trying to say here.
You're saying that even after this patch there will be a bunch of
places where pstate passed is NULL, but that's another issue itself
and may not be addressed within this patch?

This patch does not show how the error reports are influenced for
DefineType() and AlterType().

It does now that the `make check` failures have been fixed. However,
it doesn't appear where you would expect it to. For instance:

```
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed_table.out
 ERROR:  cannot rename column of typed table
 ALTER TABLE persons ALTER COLUMN name TYPE varchar;
 ERROR:  cannot alter column type of typed table
+LINE 1: ALTER TABLE persons ALTER COLUMN name TYPE varchar;
+                                         ^
 CREATE TABLE stuff (id int);
 ALTER TABLE persons INHERIT stuff;
 ERROR:  cannot change inheritance of typed table
```

Should we add more tests in specific places here?

This reminds as well that there is little coverage for many error
paths of DefineDomain(), with some paths actually modified in this
patch:
https://coverage.postgresql.org/src/backend/commands/typecmds.c.gcov.html
I'd suggest to do something about that, while on it, to check that the
parser issues a location on error.
--

Sure. I did add some tests to domain.sql. These tests check for almost
all parser_errposition() calls in DefineDomain.

The only thing I failed to check is "exclusion constraints not
possible for domains". Creating a domain with this type of contrians
fails earlier than DefineDomain.

Example:
```
db1=# create domain dbad as int CHECK(EXCLUDE (c) );
ERROR: column "c" does not exist
```
I'm not sure if this is possible to check.

--
Best regards,
Kirill Reshke

Looks like v4 fails on windows, PFA v5.

Sorry for the noise, I hope Cirrus CI will like this version.

--
Best regards,
Kirill Reshke

hi.

extensive test for
ATExecAddOf
DefineType
ATPrepAlterColumnType
ATExecAlterColumnType
DefineDomain
AlterType
transformAlterTableStmt

only AlterType, ATExecAlterColumnType function code change no tests.
AlterType doesn't have location info, can not print it out.
ATExecAlterColumnType is unreachable, because ATPrepAlterColumnType
catched most of the error.

especially extensive tests for DefineDomain.
AlterDomainAddConstraint related error case, i created another thread
(refactor AlterDomainAddConstraint (alter domain add constraint))

On 2024-Dec-06, jian he wrote:

From 6bf657c3b62b7460b317c42ce2f4fa0988acf1a0 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Fri, 6 Dec 2024 16:37:18 +0800
Subject: [PATCH v6 1/1] print out error position for some DDL command

doing this by passing the source_string to the existing ParseState
or by making a new ParseState passing source_string to it.

With this patch, the following functions will printout the error position for certain error cases.

ATExecAddOf
DefineType
ATPrepAlterColumnType
ATExecAlterColumnType
DefineDomain
AlterType
transformAlterTableStmt

I think it would make more sense to write the commit message in terms of
the DDL commands that now report error position, than the C functions.
Such a list of commands does not need to be exhaustive; a
representative-enough sample probably suffices.

@@ -943,11 +942,13 @@ DefineDomain(CreateDomainStmt *stmt)

if (constr->is_no_inherit)
ereport(ERROR,
-							errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-							errmsg("not-null constraints for domains cannot be marked NO INHERIT"));
+							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+							 errmsg("not-null constraints for domains cannot be marked NO INHERIT"),
+							 parser_errposition(pstate, constr->location)));

Once upon a time, ereport() was a simpler macro that did not
use variadic arguments. Back then, the list of functions embedded in it
(errcode, errmsg etc) were forced to be in an additional level of
parentheses so that the macro would work at all (IIRC failure to do that
resulted in strange compile-time problems). This is why a majority of
code is written in the style with those parens. But commit e3a87b4991cc
changed ereport to use __VA_ARGS__, so the auxiliary functions are
actual arguments to errstart() -- which means that the parentheses
you're adding here are unnecessary and discouraged. Just add the
parser_errposition() call and it'll be fine.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"XML!" Exclaimed C++. "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
https://burningbird.net/the-parable-of-the-languages/

Thank you for reviewing this!

On Fri, 6 Dec 2024 at 19:01, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I think it would make more sense to write the commit message in terms of
the DDL commands that now report error position, than the C functions.
Such a list of commands does not need to be exhaustive; a
representative-enough sample probably suffices.

Hi! I fixed the commit message as suggested.

Once upon a time, ereport() was a simpler macro that did not
use variadic arguments. Back then, the list of functions embedded in it
(errcode, errmsg etc) were forced to be in an additional level of
parentheses so that the macro would work at all (IIRC failure to do that
resulted in strange compile-time problems). This is why a majority of
code is written in the style with those parens. But commit e3a87b4991cc
changed ereport to use __VA_ARGS__, so the auxiliary functions are
actual arguments to errstart() -- which means that the parentheses
you're adding here are unnecessary and discouraged. Just add the
parser_errposition() call and it'll be fine.

Should be fixed in v7.

--
Best regards,
Kirill Reshke

On Mon, Dec 09, 2024 at 12:50:20PM +0500, Kirill Reshke wrote:

Should be fixed in v7.

+create domain d_fail as int4 constraint cc check(values > 1) deferrable;
+ERROR:  specifying constraint deferrability not supported for domains
+LINE 1: ...in d_fail as int4 constraint cc check(values > 1) deferrable...

I would suggest to split the patch into two pieces for clarity, based
on the fact that your v7 patch is doing more than one thing at the
same time:
- Introduce new tests for the new coverage (domain, CREATE TABLE OF,
ALTER TABLE flavors) in a first patch.
- Introduce the ParseStates in these new code paths in a second patch.

By structuring things this way, it is possible to see what kind of
difference related to the new ParseStates is introduced, based on the
new test coverage introduced in the first patch.

This makes also the whole review easier.

+ALTER TABLE itest4 ALTER COLUMN c ADD GENERATED ALWAYS AS IDENTITY;
-- error, column c does not exists

Typo here: s/exists/exist/.
--
Michael

On Tue, 10 Dec 2024 at 08:28, Michael Paquier <michael@paquier.xyz> wrote:

I would suggest to split the patch into two pieces for clarity, based
on the fact that your v7 patch is doing more than one thing at the
same time:
- Introduce new tests for the new coverage (domain, CREATE TABLE OF,
ALTER TABLE flavors) in a first patch.
- Introduce the ParseStates in these new code paths in a second patch.

By structuring things this way, it is possible to see what kind of
difference related to the new ParseStates is introduced, based on the
new test coverage introduced in the first patch.

This makes also the whole review easier.

Ok. Sure.

Typo here: s/exists/exist/.

Fixed, Thank you

PFA v8.

--
Best regards,
Kirill Reshke

add parser_errposition to some places in
transformTableConstraint, transformColumnDefinition
where v8 didn't.

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

add parser_errposition to some places in
transformTableConstraint, transformColumnDefinition
where v8 didn't.

I'm not loving the idea of cons'ing up ParseStates in random places in
tablecmds.c. I think we ought to fix things so that the one made in
standard_ProcessUtility is passed down to all these places, replacing
ad-hoc queryString and queryEnv parameters.

Eventually we might want to make ProcessUtility's callers pass down
a pstate, but that would be a whole other area of invasive changes,
so I think we should leave that idea for later. Right now though,
it seems reasonable to change AlterTableUtilityContext to replace
the queryString and queryEnv fields with a ParseState carrying that
info --- and, perhaps, someday saving us from adding more ad-hoc
fields there.

regards, tom lane

On Thu, Dec 12, 2024 at 4:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

add parser_errposition to some places in
transformTableConstraint, transformColumnDefinition
where v8 didn't.

I'm not loving the idea of cons'ing up ParseStates in random places in
tablecmds.c. I think we ought to fix things so that the one made in
standard_ProcessUtility is passed down to all these places, replacing
ad-hoc queryString and queryEnv parameters.

the main code change is within DefineDomain.

AlterTableUtilityContext comments says:
/* Info needed when recursing from ALTER TABLE */
so we cannot pass DefineDomain with AlterTableUtilityContext.

-DefineDomain(CreateDomainStmt *stmt)
+DefineDomain(ParseState *pstate, CreateDomainStmt *stmt)
we have to pass either ParseState or queryString to DefineDomain.

-extern ObjectAddress AlterType(AlterTypeStmt *stmt);
+extern ObjectAddress AlterType(ParseState *pstate, AlterTypeStmt *stmt);
this change not necessary, we can remove it.

but other places (listed in below),
we are passing (AlterTableUtilityContext *context) which seems ok?

-ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode)
+ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode,
+ AlterTableUtilityContext *context)

 static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
-   AlterTableCmd *cmd, LOCKMODE lockmode);
+   AlterTableCmd *cmd, LOCKMODE lockmode,
+   AlterTableUtilityContext *context);

 static ObjectAddress
 ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
-  AlterTableCmd *cmd, LOCKMODE lockmode)
+  AlterTableCmd *cmd, LOCKMODE lockmode,
+  AlterTableUtilityContext *context)

On Tue, Dec 10, 2024 at 10:38:41PM +0800, jian he wrote:

add parser_errposition to some places in
transformTableConstraint, transformColumnDefinition
where v8 didn't.

I've looked at the new tests in 0001. Here are some notes. And I've
found some mistakes and simplifications on the way.

 CREATE TYPE test_type2 AS (a int, b text);
+CREATE TABLE test_tbl2 OF xx;
+ERROR:  type "xx" does not exist
[...]
+ALTER TABLE tt0 OF tt_t_noexist;
+ERROR:  type "tt_t_noexist" does not exist

typed_table.out checks that already, so these additions bring nothing
new:
CREATE TABLE ttable1 OF nothing;
ERROR: type "nothing" does not exist

The three tests for ALTER TABLE .. SET DATA TYPE are new patterns, so
these are OK. The COLLATE case was kind of covered with CREATE
DOMAIN, but the command is different.

The ALTER TABLE .. ALTER COLUMN case for a generated column is new, so
that's OK.

CREATE TYPE (like=no_such_type) also makes sense, that's new coverage.
This query pattern is only used in expressions, float8 and float4.

+--test error report position

As of 0001, this comment is incorrect. We're not testing an error
position yet. With 0002, it would be correct. Let's just use a more
generic wording that applies to both patches.

+create domain d_fail as int4 constraint cc generated always as (2) stored;
+ERROR:  specifying GENERATED not supported for domains
+create domain d_fail as int4 constraint cc check(values > 1) no inherit;
+ERROR:  check constraints for domains cannot be marked NO INHERIT

Can be reduced to one rather than two.

+create domain d_fail as int4 constraint cc check(values > 1) deferrable;
+ERROR:  specifying constraint deferrability not supported for domains
+create domain d_fail as int4 constraint cc check(values > 1) not deferrable;
+ERROR:  specifying constraint deferrability not supported for domains
+create domain d_fail as int4 constraint cc check (value > 1) initially deferred;
+ERROR:  specifying constraint deferrability not supported for domains
+create domain d_fail as int4 constraint cc check(values > 1) initially immediate;
+ERROR:  specifying constraint deferrability not supported for domains
+create domain d_fail as int4 constraint cc check(values > 1) deferrable not deferrable ;
+ERROR:  specifying constraint deferrability not supported for domains

Testing the full set of keywords is not really interesting. So let's
just use one to make the script cheaper.

I was wondering for a few seconds about exclusion constraints, but it
requires a named constraint to trigger the error, so I've left it out
for now.

+create domain d_fail as int constraint cc REFERENCES this_table_not_exists(i);

Hmm. Funny. We don't document REFERENCES in the docs of CREATE
DOMAIN. Neither do we document GENERATED. Looks like a doc issue to
me, independent of this thread. ALTER DOMAIN uses a different parsing
clause than CREATE DOMAIN, meaning that generated columns or
references cannot be altered.. It looks like there's quite a bit more
going on here. The fact that we don't have tests for these patterns
authorized by the parser should be tracked anyway, so let's add them
for now. This should be looked at on a separate thread.

For now, I've applied the new tests. Let's move on with the additions
of 0002, and see if these are good to have or not (noticed Tom's
comments about the type paths, of course).
--
Michael

On Thu, Dec 12, 2024 at 10:08:04AM +0800, jian he wrote:

On Thu, Dec 12, 2024 at 4:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm not loving the idea of cons'ing up ParseStates in random places in
tablecmds.c. I think we ought to fix things so that the one made in
standard_ProcessUtility is passed down to all these places, replacing
ad-hoc queryString and queryEnv parameters.

the main code change is within DefineDomain.

[ .. more comments from Jian .. ]

Yeah, I'm not much a fan of some of the changes of tablecmds.c, that
makes the whole stack more complicated than it should.

ProcessUtilitySlow() passing down a ParseState to DefineDomain() and
AlterType() is more consistent and a good idea IMO.

I think that the patch should be split a bit more. Even if different
areas of the code are touched, there's more than one idea of how to
create this information for the error position, so each piece could be
committed separately with changes showing up as diffs in the
regression tests.
--
Michael

On Thu, Dec 12, 2024 at 10:29 AM Michael Paquier <michael@paquier.xyz> wrote:

I think that the patch should be split a bit more. Even if different
areas of the code are touched, there's more than one idea of how to
create this information for the error position, so each piece could be
committed separately with changes showing up as diffs in the
regression tests.
--

I've split it into two patches, one for CREATE DOMAIN only. one for
another DDL command.

0001:
I am using DefineDomain(ParseState *pstate, CreateDomainStmt *stmt) for now.
we can also pass querystring or another struct.

0002:
passing AlterTableUtilityContext for some ALTER TABLE subroutine.
add parser_errposition to some existing `ereport(ERROR` places.
-------------------
you mentioned ALTER DOMAIN, I have further simplified it at
/messages/by-id/CACJufxG0N_WLfk-NC_k5w6vv26qLvXupbHvnkKtc2npftJQicQ@mail.gmail.com

create domain d_fail as int constraint cc REFERENCES this_table_not_exists(i);
like this command will fail, so we don't need to change
create_domain.sgml synopsis section
?

On Thu, Dec 12, 2024 at 12:38:06PM +0800, jian he wrote:

I am using DefineDomain(ParseState *pstate, CreateDomainStmt *stmt) for now.
we can also pass querystring or another struct.

All the changes in the alternate outputs for collate are always tricky
to track. As far as I can see, you've not missed a spot. So applied
this one.

0002:
passing AlterTableUtilityContext for some ALTER TABLE subroutine.
add parser_errposition to some existing `ereport(ERROR` places.
-------------------
you mentioned ALTER DOMAIN, I have further simplified it at
/messages/by-id/CACJufxG0N_WLfk-NC_k5w6vv26qLvXupbHvnkKtc2npftJQicQ@mail.gmail.com

-        likeType = typenameType(NULL, defGetTypeName(likeTypeEl), NULL);
+        likeType = typenameType(pstate, defGetTypeName(likeTypeEl), NULL);

The only test impacted by this change is the CREATE TYPE (LIKE) in
float8. It seems like this should be separated as a change of its own
as it impacts its own command.

For the rest, we're just manipulating ATExecAddOf(),
ATPrepAlterColumnType() and ATExecAlterColumnType(). FWIW, I'm
feeling annoyed with these new make_parsestate() calls, also knowing
that we do it twice for the prep and exec parts of AlterColumnType.
Perhaps that's fine at the end, that's just an increase of calls to
make_parsestate(), still...

like this command will fail, so we don't need to change
create_domain.sgml synopsis section?

Yep, right. I was getting the impression that it would be possible to
have these ones go through with the parser allowed them when I looked
at that last Thursday. Will double-check to be sure.
--
Michael

On Mon, Dec 16, 2024 at 2:15 PM Michael Paquier <michael@paquier.xyz> wrote:

-        likeType = typenameType(NULL, defGetTypeName(likeTypeEl), NULL);
+        likeType = typenameType(pstate, defGetTypeName(likeTypeEl), NULL);

The only test impacted by this change is the CREATE TYPE (LIKE) in
float8. It seems like this should be separated as a change of its own
as it impacts its own command.

For the rest, we're just manipulating ATExecAddOf(),
ATPrepAlterColumnType() and ATExecAlterColumnType(). FWIW, I'm
feeling annoyed with these new make_parsestate() calls, also knowing
that we do it twice for the prep and exec parts of AlterColumnType.
Perhaps that's fine at the end, that's just an increase of calls to
make_parsestate(), still...

I've removed code changes related to ATExecAddOf.
ATPrepAlterColumnType will catch most of the error, so
ATExecAlterColumnType related change is not necessary.

i've split into 3 patches, feel free to merge them in any way.
v12-0001: add error position for ATPrepAlterColumnType.

v12-0002: add error position for these 3 functions:
transformColumnDefinition, transformAlterTableStmt, transformTableConstraint.

v12-0003: add error position for these 2 functions:
DefineType, transformOfType

like this command will fail, so we don't need to change
create_domain.sgml synopsis section?

Yep, right. I was getting the impression that it would be possible to
have these ones go through with the parser allowed them when I looked
at that last Thursday. Will double-check to be sure.
--

v6-0001-print-out-error-position-for-some-DDL-command.patch
at /messages/by-id/CACJufxGQtN2YzecMx=Odk8+kCTeoN7f=M_kM5e05UDW1H1PbkA@mail.gmail.com
have extensive tests.

On Mon, Dec 16, 2024 at 05:25:45PM +0800, jian he wrote:

i've split into 3 patches, feel free to merge them in any way.
v12-0001: add error position for ATPrepAlterColumnType.

For this one, why don't you do the same for undefined columns and
USING with generated columns at least? This looks half-baked.

v12-0002: add error position for these 3 functions:
transformColumnDefinition, transformAlterTableStmt, transformTableConstraint.

 ERROR:  column "c" of relation "itest4" does not exist
+LINE 1: ALTER TABLE itest4 ALTER COLUMN c ADD GENERATED ALWAYS AS ID...
+                                              ^

This one is kind of confusing? The part that matters for the error is
the column that does not exist, not the ADD GENERATED.

 ERROR:  conflicting NO INHERIT declarations for not-null constraints on column "a"
+LINE 1: ..._tbl_fail (a int generated by default as identity not null n...
+                                                             ^

This one also, is kind of hard-ish to act on..

v12-0003: add error position for these 2 functions:
DefineType, transformOfType

This one has been applied as of 0f23dedc9176.
--
Michael