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 %s to 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 %s to 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 the table 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers