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

On Wed, Jul 16, 2014 at 1:10 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
>
> 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.
>

Actually the AT processing ALTER TABLE subcommands in three phases:

1) Prepare the subcommands (ATPrepCmd for each subcommand)

2) Rewrite Catalogs (update system catalogs): in this phase the ATExecCmd
is called to run the properly checks and change the system catalog.

3) Rewrite Tables (if needed of course): this phase rewrite the relation as
needed (we force it setting tab>rewrite = true in ATPrepCmd)

And if we have just one subcommand (the case of SET (UN)LOGGED) then will
be exists just one entry in wqueue .

Anyway I think all is ok now. Is this ok for you?

> > + 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.
>

Sorry, I didn't understand what you meant.

> > @@ -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.
>

Fixed.

> >
> > /*
> > + * 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 :)
>

Fixed.

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

Fixed.

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

Fixed.

> > + */
> > +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.)
>

Fixed.

> > +/*
> > + * 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.
>

Fixed.

> > + *
> > + * Throws an exception when:
>
> "when: if" is duplicated.
>
> Throws exceptions:
>

Fixed.

> > + *
> > + * - 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.
>

Fixed. (learning a lot about English grammar... thanks)

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

Fixed.

> > + */
> > +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.)
>

The second argument of "systable_beginscan" is the Oid of index to
conditionally use. If we search by "conrelid" we have an index on pg_proc
for this column, but "confrelid" don't has an index. See another use case
in src/backend/commands/vacuum.c:812

Added a comment.

> > + 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...
>

Fixed.

> > + 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 */
>
> ...
>

Fixed.

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

Fixed.

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

Fixed.

> > +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
>

Fixed.

> > +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
>
> ...
>

Fixed.

> > +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
>

Fixed.

> I think we are almost there :)
>

Yeah... thanks a lot for your help.

Att,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello