Horton

Thread

Commits

  1. Print out error position for some ALTER TABLE ALTER COLUMN type

  2. Print out error position for some more DDLs

  3. Print out error position for CREATE DOMAIN

  4. Add some regression tests for missing DDL patterns

  5. Re-implement the ereport() macro using __VA_ARGS__.

  1. Pass ParseState as down to utility functions.

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-28T09:31:27Z

    Hi hackers!
PFA patch fixing a number of places where typenameType called with NULL pstate.

=== motivation.

Per discussion in a nearby thread for `CREATE SCHEMA ... CREATE DOMAIN
support`. Suggested by Jian He & Tom Lane.

On Thu, 28 Nov 2024 at 10:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Kirill Reshke <reshkekirill@gmail.com> writes:
> > On Wed, 27 Nov 2024 at 23:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> We've fixed a few utility statements so that they can receive
> >> a passed-down ParseState, but not DefineDomain.
>
> > PFA as an independent patch then. Or should we combine these two into one?
>
> No, I don't think this should be part of the patch discussed in this
> thread.
>
> It feels rather random to me to be fixing only DefineDomain;
> I'm sure there's more in the same vein.  I'd like to see a
> patch with a scope along the lines of "fix everything reachable
> within CREATE SCHEMA" or perhaps "fix all calls of typenameType".
> (A quick grep shows that an outright majority of the callers of that
> are passing null ParseState.  I didn't look to see if any of them
> have a good excuse beyond "we didn't do the plumbing work".)
>
>                         regards, tom lane


I chosed "fix all calls of typenameType" way.

I searched for typenameType(NULL pattern within sources and changed to
pass pstate where appropriate. This is AlterType, DefineDomain and
transformOfType cases. There are 2 more usages of this pattern left,
inside ATExecAlterColumnType & ATExecAddOf, which I dont think need to
be addressed (cure worse than disease).

=== examples
1) CREATE TYPE.
before:

```
db2=# create type int8alias3 (
    input = int8alias2in,
    output = int8alias2out,
    like = int82
);
ERROR:  type "int82" does not exist
db2=# ^C
```

after:

```
db2=# create type int8alias3 (
    input = int8alias2in,
    output = int8alias2out,
    like = int82
);
ERROR:  type "int82" does not exist
LINE 4:     like = int82
                   ^
db2=#
```

2) TABLE of TYPENAME case

before:

```
db2=# CREATE TABLE example OF mytype2 (PRIMARY KEY (some_id));
ERROR:  type "mytype2" does not exist
db2=#

```

after:

```
db2=# CREATE TABLE example OF mytype2 (PRIMARY KEY (some_id));
ERROR:  type "mytype2" does not exist
LINE 1: CREATE TABLE example OF mytype2 (PRIMARY KEY (some_id));
                                ^
```

3) CREATE DOMAIN - analogous.

==== testing

By-hand. Let me know if we can check this any other way.


-- 
Best regards,
Kirill Reshke

  2. Re: Pass ParseState as down to utility functions.

    jian he <jian.universality@gmail.com> — 2024-11-30T12:36:00Z

    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?

  3. Re: Pass ParseState as down to utility functions.

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-04T10:31:47Z

    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

  4. Re: Pass ParseState as down to utility functions.

    Michael Paquier <michael@paquier.xyz> — 2024-12-05T06:45:41Z

    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

  5. Re: Pass ParseState as down to utility functions.

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-05T12:22:34Z

    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

  6. Re: Pass ParseState as down to utility functions.

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-05T13:13:30Z

    Looks like v4 fails on windows, PFA v5.

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


-- 
Best regards,
Kirill Reshke

  7. Re: Pass ParseState as down to utility functions.

    jian he <jian.universality@gmail.com> — 2024-12-06T08:41:49Z

    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))

  8. Re: Pass ParseState as down to utility functions.

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-12-06T14:01:03Z

    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/

  9. Re: Pass ParseState as down to utility functions.

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-09T07:50:20Z

    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

  10. Re: Pass ParseState as down to utility functions.

    Michael Paquier <michael@paquier.xyz> — 2024-12-10T03:28:41Z

    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

  11. Re: Pass ParseState as down to utility functions.

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-10T09:02:56Z

    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

  12. Re: Pass ParseState as down to utility functions.

    jian he <jian.universality@gmail.com> — 2024-12-10T14:38:41Z

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

  13. Re: Pass ParseState as down to utility functions.

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-11T20:48:57Z

    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

  14. Re: Pass ParseState as down to utility functions.

    jian he <jian.universality@gmail.com> — 2024-12-12T02:08:04Z

    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)

  15. Re: Pass ParseState as down to utility functions.

    Michael Paquier <michael@paquier.xyz> — 2024-12-12T02:19:13Z

    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

  16. Re: Pass ParseState as down to utility functions.

    Michael Paquier <michael@paquier.xyz> — 2024-12-12T02:29:03Z

    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

  17. Re: Pass ParseState as down to utility functions.

    jian he <jian.universality@gmail.com> — 2024-12-12T04:38:06Z

    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
https://postgr.es/m/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
?

  18. Re: Pass ParseState as down to utility functions.

    Michael Paquier <michael@paquier.xyz> — 2024-12-16T06:15:27Z

    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
> https://postgr.es/m/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

  19. Re: Pass ParseState as down to utility functions.

    jian he <jian.universality@gmail.com> — 2024-12-16T09:25:45Z

    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 https://postgr.es/m/CACJufxGQtN2YzecMx=Odk8+kCTeoN7f=M_kM5e05UDW1H1PbkA@mail.gmail.com
have extensive tests.

  20. Re: Pass ParseState as down to utility functions.

    Michael Paquier <michael@paquier.xyz> — 2024-12-25T09:28:52Z

    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

  21. Re: Pass ParseState as down to utility functions.

    jian he <jian.universality@gmail.com> — 2024-12-27T07:01:58Z

    On Wed, Dec 25, 2024 at 5:29 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.
>

I think I understand what you mean.
please check attached for changes within ATPrepAlterColumnType


> > 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.
>
I agree this is confusing.

while looking at 0002:
errmsg("conflicting NO INHERIT declarations for not-null constraints
on column \"%s\"", column->colname)
add parser_errposition
will not be very helpful. i think we need an errhint.

for example:
create table notnull_tbl_fail (a int primary key constraint foo not
null no inherit);
ERROR:  conflicting NO INHERIT declarations for not-null constraints
on column "a"
the error message didn't explicitly say that the primary key imply a
not-null inherit constraint.

Maybe we can change to
errmsg("conflicting NO INHERIT declarations for not-null constraints
on column \"%s\"", column->colname),
errhint("specified primary key or identity sequence imply an inherited
not-null constraint will be created")

what do you think?

  22. Re: Pass ParseState as down to utility functions.

    Michael Paquier <michael@paquier.xyz> — 2025-01-27T04:57:23Z

    On Fri, Dec 27, 2024 at 03:01:58PM +0800, jian he wrote:
> please check attached for changes within ATPrepAlterColumnType

Sorry for the late reply.  This is more complete.  Two strings are
more complex after doing the coerce_to_target_type(), but honestly I
am not these are worth having a parser_errposition() as they also have
an errhint().

> Maybe we can change to
> errmsg("conflicting NO INHERIT declarations for not-null constraints
> on column \"%s\"", column->colname),
> errhint("specified primary key or identity sequence imply an inherited
> not-null constraint will be created")
> 
> what do you think?

Hmm.  Not sure that this is an improvement.  Will think a bit more
about it.
--
Michael