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.
>
> 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.
> - 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.
David J.
