On Thu, 5 Dec 2024 at 11:45, Michael Paquier <mich...@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.universal...@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
v4-0001-print-out-error-position-for-some-DDL-command.patch
Description: Binary data
