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

On Wed, Jul 16, 2014 at 3:13 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> 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.
>

(sorry by the noise)

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