Re: Pass ParseState as down to utility functions.

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