On 2024-05-16 17:47 +0200, David G. Johnston wrote:
> On Wed, May 15, 2024 at 8:46 AM Robert Haas <robertmh...@gmail.com> wrote:
>
> > On Thu, Apr 4, 2024 at 12:41 AM Erik Wienhold <e...@ewie.name> wrote:
> > > Thanks, fixed in v4. Looks like American English prefers that comma and
> > > it's also more common in our docs.
> >
> > Reviewing this patch:
> >
> > - Creates a <firstterm>typed table</firstterm>, which takes its
> > - structure from the specified composite type (name optionally
> > - schema-qualified). A typed table is tied to its type; for
> > - example the table will be dropped if the type is dropped
> > - (with <literal>DROP TYPE ... CASCADE</literal>).
> > + Creates a <firstterm>typed table</firstterm>, which takes its
> > structure
> > + from an existing (name optionally schema-qualified) stand-alone
> > composite
> > + type (i.e., created using <xref linkend="sql-createtype"/>) though
> > it
> > + still produces a new composite type as well. The table will have
> > + a dependency on the referenced type such that cascaded alter and
> > drop
> > + actions on the type will propagate to the table.
> >
> > It would be better if this diff didn't reflow the unchanged portions
> > of the paragraph.
Right. I now reformatted it so that first line remains unchanged. But
the rest of the para is still a complete rewrite.
> > I agree that it's a good idea to mention that the table must have been
> > created using CREATE TYPE .. AS here, but I disagree with the rest of
> > the rewording in this hunk. I think we could just add "creating using
> > CREATE TYPE" to the end of the first sentence, with an xref, and leave
> > the rest as it is.
>
>
>
> > I don't see a reason to mention that the typed
> > table also spawns a rowtype; that's just standard CREATE TABLE
> > behavior and not really relevant here.
>
>
> I figured it wouldn't be immediately obvious that the system would create a
> second type with identical structure. Of course, in order for SELECT tbl
> FROM tbl; to work it must indeed do so. I'm not married to pointing out
> this dynamic explicitly though.
>
>
> > And I don't understand what the
> > rest of the rewording does for us.
> >
>
> It calls out the explicit behavior that the table's columns can change due
> to actions on the underlying type. Mentioning this unique behavior seems
> worth a sentence.
>
>
> > <para>
> > - When a typed table is created, then the data types of the
> > - columns are determined by the underlying composite type and are
> > - not specified by the <literal>CREATE TABLE</literal> command.
> > + A typed table always has the same column names and data types as the
> > + type it is derived from, and you cannot specify additional columns.
> > But the <literal>CREATE TABLE</literal> command can add defaults
> > - and constraints to the table and can specify storage parameters.
> > + and constraints to the table, as well as specify storage parameters.
> > </para>
> >
> > I don't see how this is better.
> >
>
> I'll agree this is more of a stylistic change, but mainly because the talk
> about data types reasonably implies the other items the patch explicitly
> mentions - names and additional columns.
I prefer David's changes to both paras because right now the details of
typed tables are spread over the respective CREATE and ALTER commands
for types and tables. Or maybe we should add those details to the
existing "Typed Tables" section at the very end of CREATE TABLE?
> > - errmsg("type %s is not a composite type",
> > + errmsg("type %s is not a stand-alone composite type",
> >
> > I agree with Peter's complaint that people aren't going to understand
> > what a stand-alone composite type means when they see the revised
> > error message; to really help people, we're going to need to do better
> > than this, I think.
> >
> >
> We have a glossary.
>
> That said, leave the wording as-is and add a conditional hint: The
> composite type must not also be a table.
It's now a separate error message (like I already had in v1) which
states that the specified type must not be a row type of another table
(based on Peter's feedback). And the hint directs the user to CREATE
TYPE.
In passing, I also quoted the type name in the existing error message
for consistency. I saw that table names etc. are already quoted in
other error messages.
--
Erik
>From 575bfec362bde5bf77ccb793bd8e2d78679c062f Mon Sep 17 00:00:00 2001
From: Erik Wienhold <e...@ewie.name>
Date: Fri, 8 Mar 2024 04:21:56 +0100
Subject: [PATCH v5] Document that typed tables rely on CREATE TYPE
CREATE TABLE OF requires a stand-alone composite type that is not the
row type of another table. Clarify that with a reference to CREATE TYPE
in the docs. Also report a separate error message in this case.
Reworded docs provided by David G. Johnston.
---
doc/src/sgml/ref/create_table.sgml | 16 ++++++++--------
src/backend/commands/tablecmds.c | 11 +++++++++--
src/test/regress/expected/typed_table.out | 7 ++++++-
src/test/regress/sql/typed_table.sql | 4 ++++
4 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index f19306e776..11458be9cf 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -249,18 +249,18 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<listitem>
<para>
Creates a <firstterm>typed table</firstterm>, which takes its
- structure from the specified composite type (name optionally
- schema-qualified). A typed table is tied to its type; for
- example the table will be dropped if the type is dropped
- (with <literal>DROP TYPE ... CASCADE</literal>).
+ structure from an existing (name optionally schema-qualified) stand-alone
+ composite type (i.e., created using <xref linkend="sql-createtype"/>)
+ though it still produces a new composite type as well. The table will
+ have a dependency on the referenced type such that cascaded alter and
+ drop actions on the type will propagate to the table.
</para>
<para>
- When a typed table is created, then the data types of the
- columns are determined by the underlying composite type and are
- not specified by the <literal>CREATE TABLE</literal> command.
+ A typed table always has the same column names and data types as the
+ type it is derived from, and you cannot specify additional columns.
But the <literal>CREATE TABLE</literal> command can add defaults
- and constraints to the table and can specify storage parameters.
+ and constraints to the table, as well as specify storage parameters.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 313c782cae..40c7077ecd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6935,11 +6935,18 @@ check_of_type(HeapTuple typetuple)
* the type before the typed table creation/conversion commits.
*/
relation_close(typeRelation, NoLock);
+
+ if (!typeOk)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("type \"%s\" must not be a row type of another table",
+ format_type_be(typ->oid)),
+ errhint("must be a stand-alone composite type created with CREATE TYPE")));
}
- if (!typeOk)
+ else
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("type %s is not a composite type",
+ errmsg("type \"%s\" is not a composite type",
format_type_be(typ->oid))));
}
diff --git a/src/test/regress/expected/typed_table.out b/src/test/regress/expected/typed_table.out
index 2e47ecbcf5..0ddf839012 100644
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed_table.out
@@ -89,8 +89,13 @@ drop cascades to function get_all_persons()
drop cascades to table persons2
drop cascades to table persons3
CREATE TABLE persons5 OF stuff; -- only CREATE TYPE AS types may be used
-ERROR: type stuff is not a composite type
+ERROR: type "stuff" must not be a row type of another table
+HINT: must be a stand-alone composite type created with CREATE TYPE
DROP TABLE stuff;
+CREATE TYPE simple AS ENUM ('a');
+CREATE TABLE of_simple OF simple; -- not a composite type
+ERROR: type "simple" is not a composite type
+DROP TYPE simple;
-- implicit casting
CREATE TYPE person_type AS (id int, name text);
CREATE TABLE persons OF person_type;
diff --git a/src/test/regress/sql/typed_table.sql b/src/test/regress/sql/typed_table.sql
index 9ef0cdfcc7..d9b9398857 100644
--- a/src/test/regress/sql/typed_table.sql
+++ b/src/test/regress/sql/typed_table.sql
@@ -50,6 +50,10 @@ CREATE TABLE persons5 OF stuff; -- only CREATE TYPE AS types may be used
DROP TABLE stuff;
+CREATE TYPE simple AS ENUM ('a');
+CREATE TABLE of_simple OF simple; -- not a composite type
+DROP TYPE simple;
+
-- implicit casting
--
2.45.1
