Adding type info etc for inheritance errmsg: "child table is missing column ..."
Hey Postgres team!
I've been gleefully using the inheritance feature of Postgres for the last
6 months or so, and it's really great! I especially like how easily you
can ALTER TABLE foo INHERIT bar, and get helpful error messages about what
columns need to be there before the inherit can take place.
One thing that seemed helpful to me is if it not only told you that you're
missing the attribute, but also told you the type, and perhaps even the
constraints, so you can easily make the column match up with the one you're
inheriting. Otherwise you may add the wrong type of column and end up with
a "column is the wrong type" error.
The attached patch is my initial attempt at adding the type, making the
error message read e.g.:
ERROR: child table is missing column "name" text
I'm sure it needs work (in particular I borrowed a lot of the get-type-name
logic from getTypeOutputInfo() so probably needs a factor), and I'd
ultimately love for it to list NOT NULL, DEFAULTs and other constraints to
make it easier to prepare a table to inherit from another.
Thoughts / suggestions? Does this seem useful to you guys?
Best,
Ryan
Attachments:
0001-child-table-is-missing-attribute-show-type.patchapplication/octet-stream; name=0001-child-table-is-missing-attribute-show-type.patchDownload
From d738e4ca815d89eaa5897028335e319a5024a901 Mon Sep 17 00:00:00 2001
From: ryanfmurphy <ryanfmurphy@gmail.com>
Date: Sat, 7 Jan 2017 09:39:49 -0600
Subject: [PATCH] child table is missing attribute: show type
just a preliminary patch for discussion
would like to show type name and constraints
for convenience in making inherited structures consistent
---
src/backend/commands/tablecmds.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1c219b0..9f6ca2d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10893,12 +10893,17 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
CatalogUpdateIndexes(attrrel, tuple);
heap_freetuple(tuple);
}
- else
+ else // child table is missing column - print error msg
{
+ // get the type to display to the user
+ HeapTuple typeTuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(attribute->atttypid));
+ // #todo #fixme - factor into function? lots of logic copied from getTypeOutputInfo in src/backend/utils/cache/lsyscache.c
+ Form_pg_type attributeType = (Form_pg_type) GETSTRUCT(typeTuple);
+
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("child table is missing column \"%s\"",
- attributeName)));
+ errmsg("child table is missing column \"%s\" %s",
+ attributeName, NameStr(attributeType->typname))));
}
}
--
2.5.4 (Apple Git-61)
Ryan Murphy <ryanfmurphy@gmail.com> writes:
The attached patch is my initial attempt at adding the type, making the
error message read e.g.:
ERROR: child table is missing column "name" text
"... column "name" of type text", perhaps? Does not read very nicely
as is.
I'm sure it needs work (in particular I borrowed a lot of the get-type-name
logic from getTypeOutputInfo() so probably needs a factor),
The approved way to do this is with format_type_be(), or
format_type_with_typemod() if you want to include typmod info, which
I think you probably do for the intended use-case.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
The approved way to do this is with format_type_be(), or
format_type_with_typemod() if you want to include typmod info, which
I think you probably do for the intended use-case.regards, tom lane
Thanks Tom, I'll redo the patch using one of those, and get back to you
guys.
Thanks Tom, I'll redo the patch using one of those, and get back to you
guys.
So I tried using format_type_with_typemod() thinking that the "typemod
info" meant things like NOT NULL, DEFAULT etc. It builds and includes the
plain type but not all that stuff. E.g.
user=# alter table temp inherit entity;
ERROR: child table is missing column "id" uuid
when I was hoping for
user=# alter table temp inherit entity;
ERROR: child table is missing column "id" uuid default uuid_generate_v1mc()
Is there an easy way to get the string that includes all those additional
constraints/defaults etc? I noticed that defaults seem to be stored in the
Form_pg_attrdef struct defined in src/include/catalog/pg_attrdef.h, and
haven't yet seen how constraints like NOT NULL are handled.
Thanks,
Ryan
Ryan Murphy <ryanfmurphy@gmail.com> writes:
So I tried using format_type_with_typemod() thinking that the "typemod
info" meant things like NOT NULL, DEFAULT etc.
No, it means atttypmod, which stores info like the max length for
varchar(n).
when I was hoping for
user=# alter table temp inherit entity;
ERROR: child table is missing column "id" uuid default uuid_generate_v1mc()
Is there an easy way to get the string that includes all those additional
constraints/defaults etc?
No, and TBH I would vote strongly against including that much detail in
this error message anyway. That info could be indefinitely long, and it's
not especially relevant to the stated error condition --- for example, the
presence of a default is *not* relevant to whether the column matches the
parent. I'm okay with shoehorning column type into this message, but not
much more than that.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
No, and TBH I would vote strongly against including that much detail in
this error message anyway. That info could be indefinitely long, and it's
not especially relevant to the stated error condition --- for example, the
presence of a default is *not* relevant to whether the column matches the
parent. I'm okay with shoehorning column type into this message, but not
much more than that.regards, tom lane
Ok, that makes sense. How about things like NOT NULL? you get an error if
your column doesn't have that.
On 01/07/2017 08:15 PM, Tom Lane wrote:
Ryan Murphy <ryanfmurphy@gmail.com> writes:
I was hoping for
user=# alter table temp inherit entity;
ERROR: child table is missing column "id" uuid default uuid_generate_v1mc()
Is there an easy way to get the string that includes all those additional
constraints/defaults etc?No, and TBH I would vote strongly against including that much detail in
this error message anyway. That info could be indefinitely long, and it's
not especially relevant to the stated error condition --- for example, the
presence of a default is *not* relevant to whether the column matches the
parent. I'm okay with shoehorning column type into this message, but not
much more than that.
I agree.
Perhaps the ERROR message should remain as is, and a DETAIL or HINT line
could be emitted with the entire column definition (or close to it)?
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Vik Fearing <vik@2ndquadrant.fr> writes:
On 01/07/2017 08:15 PM, Tom Lane wrote:
No, and TBH I would vote strongly against including that much detail in
this error message anyway. That info could be indefinitely long, and it's
not especially relevant to the stated error condition --- for example, the
presence of a default is *not* relevant to whether the column matches the
parent. I'm okay with shoehorning column type into this message, but not
much more than that.
I agree.
Perhaps the ERROR message should remain as is, and a DETAIL or HINT line
could be emitted with the entire column definition (or close to it)?
AFAICS, what Ryan is after would be better served by a separate tool that
would produce a "diff" of the two tables' schemas. Even if we were
willing to overload this error message with everything we know about the
parent column, do you really want to fix discrepancies one column at a
time? And what about properties that can't be uniquely associated with a
single column, such as constraints covering multiple columns?
Also, I have a feeling that a suitable tool is already out there. A
moment's digging in the list archives found this thread with links to
several candidates:
/messages/by-id/561D27E7.5010906@trustport.com
and I'm pretty sure there have been other such discussions.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 01/07/2017 11:05 PM, Tom Lane wrote:
Vik Fearing <vik@2ndquadrant.fr> writes:
On 01/07/2017 08:15 PM, Tom Lane wrote:
No, and TBH I would vote strongly against including that much detail in
this error message anyway. That info could be indefinitely long, and it's
not especially relevant to the stated error condition --- for example, the
presence of a default is *not* relevant to whether the column matches the
parent. I'm okay with shoehorning column type into this message, but not
much more than that.I agree.
Perhaps the ERROR message should remain as is, and a DETAIL or HINT line
could be emitted with the entire column definition (or close to it)?AFAICS, what Ryan is after would be better served by a separate tool that
would produce a "diff" of the two tables' schemas. Even if we were
willing to overload this error message with everything we know about the
parent column, do you really want to fix discrepancies one column at a
time? And what about properties that can't be uniquely associated with a
single column, such as constraints covering multiple columns?Also, I have a feeling that a suitable tool is already out there.
Well personally, if I were trying to attach one table to another and it
didn't work, I'd use good ol' \d. Maybe that's just me.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/7/17 1:16 PM, Ryan Murphy wrote:
No, and TBH I would vote strongly against including that much detail in
this error message anyway. That info could be indefinitely long,
and it's
not especially relevant to the stated error condition --- for
example, the
presence of a default is *not* relevant to whether the column
matches the
parent. I'm okay with shoehorning column type into this message,
but not
much more than that.regards, tom lane
Ok, that makes sense. How about things like NOT NULL? you get an error
if your column doesn't have that.
Yeah, anything that we're explicitly testing for needs to be mentioned
in an error message, otherwise users will be very confused if the column
*is* in the parent but is failing some other test. Perhaps it would be
better for each test to spit out a different error message making it
clear what exactly was wrong.
Related to the other idea of seeing the problems that exist in all the
columns (instead of one column at a time), I think it'd be reasonable to
have a SRF that spit out everything you'd need to fix to allow
inheritance to be added. A schema diff won't know what specifically has
to match, but our code does.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Perhaps the ERROR message should remain as is, and a DETAIL or HINT line
could be emitted with the entire column definition (or close to it)?
I'm open to this option as well. The only parts I really care about are
the type and the NOT NULL, since those are the ones that will give you an
error if it doesn't line up with the parent. Putting it in a DETAIL or
HINT is fine with me.
AFAICS, what Ryan is after would be better served by a separate tool that
would produce a "diff" of the two tables' schemas.
Related to the other idea of seeing the problems that exist in all the
columns (instead of one column at a time), I think it'd be reasonable to
have a SRF that spit out everything you'd need to fix to allow inheritance
to be added. A schema diff won't know what specifically has to match, but
our code does.
Sure, I think that's totally true that I could just make a
Set-Returning-Function (that's what SRF stands for right?) that would
accomplish this. I'll try that path instead for now.
Thanks guys!
Ryan
Tom Lane wrote:
AFAICS, what Ryan is after would be better served by a separate tool that
would produce a "diff" of the two tables' schemas. Even if we were
willing to overload this error message with everything we know about the
parent column, do you really want to fix discrepancies one column at a
time? And what about properties that can't be uniquely associated with a
single column, such as constraints covering multiple columns?Also, I have a feeling that a suitable tool is already out there. A
moment's digging in the list archives found this thread with links to
several candidates:/messages/by-id/561D27E7.5010906@trustport.com
and I'm pretty sure there have been other such discussions.
I remember looking some time ago, and most of all the possible
candidates were either abandoned or terribly cumbersome to use. I ran
across Euler Taveira in a conference sometime later and he told me he
had the idea of working on writing such a tool. I was pleased to see
his announcement recently:
/messages/by-id/6c9d4a85-55b5-81cc-6f09-8f26a6da2072@timbira.com.br
I admit I have not looked at his code, but he had some very good ideas
for the complex cases. I think it's worth checking out what he has
done.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers