Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED - Mailing list pgsql-hackers

Heikki Linnakangas wrote:

> In Postgres internals slang, non-permanent means temporary or
> unlogged. But I agree we shouldn't expose users to that term; we use
> it in the docs, and it's not used in command names either.

Agreed.  I am going over this patch, and the last bit I need to sort out
is the wording of these messages.  I have temporarily settled on this:
ereport(ERROR,        (errcode(ERRCODE_INVALID_TABLE_DEFINITION),         errmsg("cannot change logged status of table
%sto logged",                RelationGetRelationName(rel)),         errdetail("Table %s references unlogged table %s.",
                 RelationGetRelationName(rel),                   RelationGetRelationName(relfk))));
 

Note the term "logged status" to talk about whether a table is logged or
not.  I thought about "loggedness" but I'm not sure english speakers are
going to love me for that.  Any other ideas there?

> I wonder if throwing an error is correct behavior anyway. Other
> ALTER TABLE commands just silently do nothing in similar situations,
> e.g:
> 
> lowerdb=# CREATE TABLE foo () WITH OIDS;
> CREATE TABLE
> lowerdb=# ALTER TABLE foo SET WITH OIDS;
> ALTER TABLE
> 
> But if we want to throw an error anyway, I'd suggest phrasing it
> "table foo is already unlogged"

Yeah, there is precedent for silently doing nothing.  We previously
threw warnings or notices, but nowadays even that is gone.  Throwing an
error definitely seems the wrong thing.  In the patch I have it's like
this:
ereport(ERROR,        (errcode(ERRCODE_INVALID_TABLE_DEFINITION),         errmsg("cannot change logged status of table
%sto unlogged",                RelationGetRelationName(rel)),         errdetail("Table %s is already unlogged.",
          RelationGetRelationName(rel))));
 

but I think I will just take that out as a whole and set a flag to
indicate nothing is to be done.

(This also means that setting tab->rewrite while analyzing the command
is the wrong thing to do.  Instead, the test for tab->rewrite should
have an || tab->chgLoggedness test, and we set chgLoggedness off if we
see that it's a no-op.  That way we avoid a pointless table rewrite and
a pointless error in a multi-command ALTER TABLE that has a no-op SET
LOGGED subcommand among other things.)


I changed the doc item in ALTER TABLE,
  <varlistentry>   <term><literal>SET {LOGGED | UNLOGGED}</literal></term>   <listitem>    <para>     This form changes
thetable from unlogged to logged or vice-versa     (see <xref linkend="SQL-CREATETABLE-UNLOGGED">).  It cannot be
applied    to a temporary table.    </para>   </listitem>  </varlistentry>
 

I removed the fact that it needs ACCESS EXCLUSIVE because that's already
mentioned in the introductory paragraph.  I also removed the phrase that
it requires a table rewrite, because on reading existing text I noticed
that we don't document which forms cause rewrites.  Perhaps we should
document that somehow, but adding it to only one item seems wrong.

I will post an updated patch as soon as I fix a bug I introduced in the
check for FKs.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services