Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED

Re: Fabrízio de Royes Mello 2014-07-15 <CAFcNs+pXpmfwi_rKF-cSBOHWC+E=xtsRNxicRGAY6BcmthBNKg(at)mail(dot)gmail(dot)com>
> > What about the wqueue mechanism, though? Isn't that made exactly for
> > the kind of catalog updates you are doing?
> >
>
> Works, but this mechanism create a new entry in pg_class for toast, so it's
> a little different than use the cluster_rel that generate a new
> relfilenode. The important is both mechanisms create new datafiles.

Ok, I had thought that any catalog changes in AT should be queued
using this mechanism to be executed later by ATExecCmd(). The queueing
only seems to be used for the cases that recurse into child tables,
which we don't.

> Merged both to a single ATPrepSetLoggedOrUnlogged(Relation rel, bool
> toLogged);

Thanks.

> But they aren't duplicated... the first opens "con->confrelid" and the
> other opens "con->conrelid" according "toLogged" branch.

Oh sorry. I had looked for that, but still missed it.

> I removed because they are not so useful than I was thinking before.
> Actually they just bloated our test cases.

Nod.

> > I think the tests could also use a bit more comments, notably the
> > commands that are expected to fail. So far I haven't tried to read
> > them but trusted that they did the right thing. (Though with the
> > SELECTs removed, it's pretty readable now.)
> >
>
> Added some comments.

Thanks, looks nice now.

> + SET TABLESPACE <replaceable class="PARAMETER">new_tablespace</replaceable>
> RESET ( <replaceable class="PARAMETER">storage_parameter</replaceable> [, ... ] )
> INHERIT <replaceable class="PARAMETER">parent_table</replaceable>
> NO INHERIT <replaceable class="PARAMETER">parent_table</replaceable>
> OF <replaceable class="PARAMETER">type_name</replaceable>
> NOT OF
> OWNER TO <replaceable class="PARAMETER">new_owner</replaceable>
> - SET TABLESPACE <replaceable class="PARAMETER">new_tablespace</replaceable>

That should get a footnote in the final commit message.

> @@ -2857,6 +2860,8 @@ AlterTableGetLockLevel(List *cmds)
> case AT_AddIndexConstraint:
> case AT_ReplicaIdentity:
> case AT_SetNotNull:
> + case AT_SetLogged:
> + case AT_SetUnLogged:
> cmd_lockmode = AccessExclusiveLock;
> break;
>
> @@ -3248,6 +3253,13 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
> /* No command-specific prep needed */
> pass = AT_PASS_MISC;
> break;
> + case AT_SetLogged:
> + case AT_SetUnLogged:
> + ATSimplePermissions(rel, ATT_TABLE);
> + ATPrepSetLoggedOrUnlogged(rel, (cmd->subtype == AT_SetLogged)); /* SET {LOGGED | UNLOGGED} */
> + pass = AT_PASS_MISC;
> + tab->rewrite = true;
> + break;
> default: /* oops */
> elog(ERROR, "unrecognized alter table type: %d",
> (int) cmd->subtype);
> @@ -3529,6 +3541,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
> case AT_GenericOptions:
> ATExecGenericOptions(rel, (List *) cmd->def);
> break;
> + case AT_SetLogged:
> + AlterTableSetLoggedOrUnlogged(rel, true);
> + break;
> + case AT_SetUnLogged:
> + AlterTableSetLoggedOrUnlogged(rel, false);
> + break;
> default: /* oops */
> elog(ERROR, "unrecognized alter table type: %d",
> (int) cmd->subtype);

I'd move all these next to the AT_DropCluster things like in all the
other lists.

>
> /*
> + * ALTER TABLE <name> SET {LOGGED | UNLOGGED}
> + *
> + * Change the table persistence type from unlogged to permanent by
> + * rewriting the entire contents of the table and associated indexes
> + * into new disk files.
> + *
> + * The ATPrepSetLoggedOrUnlogged function check all precondictions

preconditions (without trailing space :)

> + * to perform the operation:
> + * - check if the target table is unlogged/permanente

permanent

> + * - check if not exists a foreign key to/from other unlogged/permanent

if no ... exists

> + */
> +static void
> +ATPrepSetLoggedOrUnlogged(Relation rel, bool toLogged)
> +{
> + char relpersistence;
> +
> + relpersistence = (toLogged) ? RELPERSISTENCE_UNLOGGED :
> + RELPERSISTENCE_PERMANENT;
> +
> + /* check if is an unlogged or permanent relation */
> + if (rel->rd_rel->relpersistence != relpersistence)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("table %s is not %s",
> + RelationGetRelationName(rel),
> + (toLogged) ? "unlogged" : "permanent")));

I think this will break translation of the error message; you will
likely need to provide two full strings. (I don't know if
errmsg(toLogged ? "" : ""...) is acceptable or if you need to put a
full if() around it.)

> +/*
> + * AlterTableSetLoggedUnloggedCheckForeignConstraints: checks for Foreign Key
> + * constraints consistency when changing from unlogged to permanent or
> + * from permanent to unlogged.

checks foreign key
constraint consistency when changing relation persistence.

> + *
> + * Throws an exception when:

"when: if" is duplicated.

Throws exceptions:

> + *
> + * - if changing to permanent (toLogged = true) then checks if doesn't
> + * exists a foreign key to another unlogged table.
> + *
> + * - if changing to unlogged (toLogged = false) then checks if doesn't
> + * exists a foreign key from another permanent table.

- when changing to ... then checks in no ... exists.

> + *
> + * Self foreign keys are skipped from the check.

Self-referencing foreign keys ...

> + */
> +static void
> +AlterTableSetLoggedUnloggedCheckForeignConstraints(Relation rel, bool toLogged)
> +{
> + Relation pg_constraint;
> + HeapTuple tuple;
> + SysScanDesc scan;
> + ScanKeyData skey[1];
> +
> + /*
> + * Fetch the constraint tuple from pg_constraint.
> + */
> + pg_constraint = heap_open(ConstraintRelationId, AccessShareLock);
> +
> + /* scans conrelid if toLogged is true else scans confreld */
> + ScanKeyInit(&skey[0],
> + ((toLogged) ? Anum_pg_constraint_conrelid : Anum_pg_constraint_confrelid),
> + BTEqualStrategyNumber, F_OIDEQ,
> + ObjectIdGetDatum(RelationGetRelid(rel)));
> +
> + scan = systable_beginscan(pg_constraint,
> + ((toLogged) ? ConstraintRelidIndexId : InvalidOid), toLogged,
> + NULL, 1, skey);

This ": InvalidOid" needs a comment. (I have no idea what it does.)

> + while (HeapTupleIsValid(tuple = systable_getnext(scan)))
> + {
> + Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(tuple);
> + if (con->contype == CONSTRAINT_FOREIGN)
> + {
> + Relation relfk;
> +
> + if (toLogged)
> + {
> + relfk = relation_open(con->confrelid, AccessShareLock);
> +
> + /* skip if self FK or check if exists a FK to an unlogged table */

... same grammar fix as above...

> + if (RelationGetRelid(rel) != con->confrelid &&
> + relfk->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("table %s references unlogged table %s",
> + RelationGetRelationName(rel),
> + RelationGetRelationName(relfk))));
> + }
> + else
> + {
> + relfk = relation_open(con->conrelid, AccessShareLock);
> +
> + /* skip if self FK or check if exists a FK from a permanent table */

...

> diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
> index 22a2dd0..3e30ca8 100644
> --- a/src/test/regress/sql/alter_table.sql
> +++ b/src/test/regress/sql/alter_table.sql
> @@ -1624,3 +1624,35 @@ TRUNCATE old_system_table;
> ALTER TABLE old_system_table DROP CONSTRAINT new_system_table_pkey;
> ALTER TABLE old_system_table DROP COLUMN othercol;
> DROP TABLE old_system_table;
> +
> +-- set logged
> +CREATE UNLOGGED TABLE unlogged1(f1 SERIAL PRIMARY KEY, f2 TEXT);
> +-- check relpersistence of an unlogged table
> +SELECT relname, relpersistence, oid = relfilenode AS original_relfilenode FROM pg_class WHERE relname ~ '^unlogged1' ORDER BY 1;
> +CREATE UNLOGGED TABLE unlogged2(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES unlogged1); -- fk reference
> +CREATE UNLOGGED TABLE unlogged3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES unlogged3); -- self fk reference
> +ALTER TABLE unlogged3 SET LOGGED; -- skip self FK reference
> +ALTER TABLE unlogged2 SET LOGGED; -- fails because exists a FK to an unlogged table

...

> +ALTER TABLE unlogged1 SET LOGGED;
> +-- check relpersistence of an unlogged table after changed to permament

after changing to permament

> +SELECT relname, relpersistence, oid = relfilenode AS original_relfilenode FROM pg_class WHERE relname ~ '^unlogged1' ORDER BY 1;
> +DROP TABLE unlogged3;
> +DROP TABLE unlogged2;
> +DROP TABLE unlogged1;
> +
> +-- set unlogged
> +CREATE TABLE logged1(f1 SERIAL PRIMARY KEY, f2 TEXT);
> +-- check relpersistence of an permanent table

a permanent

> +SELECT relname, relpersistence, oid = relfilenode AS original_relfilenode FROM pg_class WHERE relname ~ '^logged1' ORDER BY 1;
> +CREATE TABLE logged2(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged1); -- fk reference
> +CREATE TABLE logged3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES logged3); -- self fk reference
> +ALTER TABLE logged1 SET UNLOGGED; -- fails because exists a FK from a permanent table

...

> +ALTER TABLE logged3 SET UNLOGGED; -- skip self FK reference
> +ALTER TABLE logged2 SET UNLOGGED;
> +ALTER TABLE logged1 SET UNLOGGED;
> +-- check relpersistence of a permanent table after changed to unlogged

after changing

> +SELECT relname, relpersistence, oid = relfilenode AS original_relfilenode FROM pg_class WHERE relname ~ '^logged1' ORDER BY 1;
> +DROP TABLE logged3;
> +DROP TABLE logged2;
> +DROP TABLE logged1;

I think we are almost there :)

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/