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

On Wed, Jul 16, 2014 at 1:10 PM, Christoph Berg <cb@df7cb.de> wrote:
>
> Re: Fabrízio de Royes Mello 2014-07-15 <CAFcNs+pXpmfwi_rKF-cSBOHWC+E=xtsRNxicRGAY6BcmthBNKg@mail.gmail.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