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

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

The previous patch (v6) has some trailing spaces... fixed.

(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