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

--
Thom

--
Thom

On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>
> > That means should I "FlushRelationBuffers(rel)" before change the
> > relpersistence?
>
> I don't think that'd help. I think what this means that you simply
> cannot change the relpersistence of the old relation before the heap
> swap is successful. So I guess it has to be something like (pseudocode):
>
> OIDNewHeap = make_new_heap(..);
> newrel = heap_open(OIDNewHeap, AEL);
>
> /*
>  * Change the temporary relation to be unlogged/logged. We have to do
>  * that here so buffers for the new relfilenode will have the right
>  * persistency set while the original filenode's buffers won't get read
>  * in with the wrong (i.e. new) persistency setting. Otherwise a
>  * rollback after the rewrite would possibly result with buffers for the
>  * original filenode having the wrong persistency setting.
>  *
>  * NB: This relies on swap_relation_files() also swapping the
>  * persistency. That wouldn't work for pg_class, but that can't be
>  * unlogged anyway.
>  */
> AlterTableChangeCatalogToLoggedOrUnlogged(newrel);
> FlushRelationBuffers(newrel);
> /* copy heap data into newrel */
> finish_heap_swap();
>
> And then in swap_relation_files() also copy the persistency.
>
>
> That's the best I can come up right now at least.
>

Isn't better if we can set the relpersistence as an argument to "make_new_heap" ?

I'm thinking to change the make_new_heap:

From:

 make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
               LOCKMODE lockmode)

To:

 make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
               LOCKMODE lockmode)

That way we can create the new heap with the appropriate relpersistence, so in the swap_relation_files also copy the persistency, of course.

And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes, because in the "finish_heap_swap" they'll be rebuild.

Thoughts?

Regards,

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

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

On Thu, Aug 21, 2014 at 5:23 AM, Christoph Berg <cb@df7cb.de> wrote:
>
> Re: Thom Brown 2014-08-20 <CAA-aLv7TeF8iM=7U7TsgL4S5Jh1a+shQ_Ny7gOrZc_g_YJ7uKA@mail.gmail.com>
> > "ERROR:  table test is not permanent"
> >
> > Perhaps this would be better as "table test is unlogged" as "permanent"
> > doesn't match the term used in the DDL syntax.
>
> I was also wondering that, but then figured that when ALTER TABLE SET
> UNLOGGED is invoked on temp tables, the error message "is not
> permanent" was correct while the apparent opposite "is unlogged" is
> wrong.
>
> Christoph
> --
> cb@df7cb.de | http://www.df7cb.de/

Thom,

Christoph is right... make no sense the message... see the example:

fabrizio=# create temp table foo();
CREATE TABLE
fabrizio=# alter table foo set unlogged;
ERROR:  table foo is unlogged

The previous message is better:

fabrizio=# create temp table foo();
CREATE TABLE
fabrizio=# alter table foo set unlogged;
ERROR:  table foo is not permanent
fabrizio=#
fabrizio=# create unlogged table foo2();
CREATE TABLE
fabrizio=# alter table foo2 set unlogged;
ERROR:  table foo2 is not permanent

Patch attached.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> BTW why is it that index_build() checks the heap's relpersistence flag
> rather than the index'?
>

I'm curious about it too... the code in src/backend/catalog/index.c is:

1975     if (heapRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED &&
1976         !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM))
1977     {

Should not to be in that way?

1975     if (indexRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED &&
1976         !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM))
1977     {

Alvaro, is this your concern? Right?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Sat, Aug 23, 2014 at 8:53 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On Sat, Aug 23, 2014 at 3:32 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Great.  Pushed.  Thanks for the patch.
> There is a typo in what has been pushed. Patch attached.
>

Thanks... I fixed that in my last patch to change 'loggedness' to 'persistence'. Attached.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Mon, Jul 21, 2014 at 9:51 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>
> On 2014-07-16 20:45:15 -0300, Fabrízio de Royes Mello wrote:
> > > The rewrite will read in the 'old' contents - but because it's done
> > > after the pg_class.relpersistence is changed they'll all not be marked
> > > as BM_PERMANENT in memory. Then the ALTER TABLE is rolled back,
> > > including the relpersistence setting. Which will unfortunately leave
> > > pages with the wrong persistency setting in memory, right?
> > >
> >
> > That means should I "FlushRelationBuffers(rel)" before change the
> > relpersistence?
>
> Did my explanation clarify the problem + possible solution sufficiently?
>

Yes, your explanation was enough. I didn't had time to working on it yet. But I hope  working on it today or tomorrow at least.

Regards,

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

- slaves sync

On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> > 2. The patch seems to think that it can sometimes be safe to change
> > the relpersistence of an existing relation.  Unless you can be sure
> > that no buffers can possibly be present in shared_buffers and nobody
> > will use an existing relcache entry to read a new one in, it's not,
> > because the buffers won't have the right BM_PERSISTENT marking.  I'm
> > very nervous about the fact that this patch seems not to have touched
> > bufmgr.c, but maybe I'm missing something.
>
> Right.  Andres pointed this out previously, and the patch was updated
> consequently.  The only remaining case in which we do that, again AFAIR,
> is for indexes, just after the table has been rewritten and just before
> the indexes are reindexed.  There should be no buffer access of the old
> indexes at that point, so there would be no buffer marked with the wrong
> flag.  Also, the table is locked access-exclusively (is that a word?),
> so no other transaction could possibly be reading buffers for its
> indexes.
>
> I pointed out, in the email just before pushing the patch, that perhaps
> we should pass down the new relpersistence flag into finish_heap_swap,
> and from there we could pass it down to reindex_index() which is where
> it would be needed.  I'm not sure it's worth the trouble, but I think we
> can still ask Fabrizio to rework that part.
>
> Maybe it is me missing something.
>
> BTW why is it that index_build() checks the heap's relpersistence flag
> rather than the index'?
>

I can rework this part if it's a real concern.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Mon, Jul 14, 2014 at 3:31 PM, Christoph Berg <cb@df7cb.de> wrote:
>
> Oh I wasn't aware of the wiki page, I had just read the old thread.
> Thanks for the pointer.
>

:-)

Thanks again for your review!

> > > > diff --git a/doc/src/sgml/ref/alter_table.sgml
> > b/doc/src/sgml/ref/alter_table.sgml
> > > > index 69a1e14..424f2e9 100644
> > > > --- a/doc/src/sgml/ref/alter_table.sgml
> > > > +++ b/doc/src/sgml/ref/alter_table.sgml
> > > > @@ -58,6 +58,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable
> > class="PARAMETER">name</replaceable>
> > > >      ENABLE REPLICA RULE <replaceable
> > class="PARAMETER">rewrite_rule_name</replaceable>
> > > >      ENABLE ALWAYS RULE <replaceable
> > class="PARAMETER">rewrite_rule_name</replaceable>
> > > >      CLUSTER ON <replaceable class="PARAMETER">index_name</replaceable>
> > > > +    SET {LOGGED | UNLOGGED}
> > > >      SET WITHOUT CLUSTER
> > > >      SET WITH OIDS
> > > >      SET WITHOUT OIDS
> > >
> > > This must not be between the two CLUSTER lines. I think the best spot
> > > would just be one line down, before SET WITH OIDS.
> >
> > Fixed.
>
> The (long) SET LOGGED paragraph is still between CLUSTER and SET
> WITHOUT CLUSTER.
>

> > > This grammar bug pops up consistently: This form *changes*...
> > >
> >
> > Fixed.
>
> Two more:
>
> + * The AlterTableChangeCatalogToLoggedOrUnlogged function perform the
> + * The AlterTableChangeIndexesToLoggedOrUnlogged function scan all indexes
>

Fixed.

> > > >                       relation_close(rel, NoLock);
> > > > +
> > > > +                     if (pass == AT_PASS_SET_LOGGED_UNLOGGED)
> > > > +
> > ATPostAlterSetLoggedUnlogged(RelationGetRelid(rel));
> > >
> > > This must be done before relation_close() which releases all locks.
>
> You didn't address that. I'm not sure about it, but either way, this
> deserves a comment on the lock level necessary.
>

Actually relation_close(rel, NoLock) don't release the locks.

See src/backend/access/heap/heapam.c:1167

> > > Moreover, I think you can get rid of that extra PASS here.
> > > AT_PASS_ALTER_TYPE has its own pass because you can alter several
> > > columns in a single ALTER TABLE statement, but you can have only one
> > > SET (UN)LOGGED, so you can to the cluster_rel() directly in
> > > AlterTableSetLoggedOrUnlogged() (unless cluster_rel() is too intrusive
> > > and would interfere with other ALTER TABLE operations in this command,
> > > no idea).
> > >
> >
> > I had some troubles here so I decided to do in that way, but I confess I'm
> > not comfortable with this implementation. Looking more carefully on
> > tablecmds.c code, at the ATController we have three phases and the third is
> > 'scan/rewrite tables as needed' so my doubt is if can I use it instead of
> > call 'cluster_rel'?
>
> I've just tried some SET (UN)LOGGED operations with altering column
> types in the same operation, that works. But:
>
> Yes, you should use the existing table rewriting machinery, or at
> least clearly document (in comments) why it doesn't work for you.
>
> Also looking at ATController, there's a wqueue mechanism to queue
> catalog updates. You should probably use this, too, or again document
> why it doesn't work for you.
>

This works... fixed!

> > > Here's the big gotcha: Just like SET LOGGED must check for outgoing
> > > FKs to unlogged tables, SET UNLOGGED must check for incoming FKs from
> > > permanent tables. This is missing.
> > >
> >
> > I don't think so... we can create an unlogged table with a FK referring to
> > a regular table...
> > ... but is not possible create a FK from a regular table referring to an
> > unlogged table:
> > ... and a FK from an unlogged table referring other unlogged table works:
> > So we must take carefull just when changing an unlogged table to a regular
> > table.
> >
> > Am I correct or I miss something?
>
> You miss the symmetric case the other way round. When changing a table
> to unlogged, you need to make sure no other permanent table is
> referencing our table.
>

Ohh yeas... sorry... you're completely correct... fixed!

> > > > +AlterTableChangeCatalogToLoggedOrUnlogged(Relation rel, Relation
> > relrelation, bool toLogged)
>
> You are using "relrelation" and "relrel". I'd change all occurrences
> to "relrelation" because that's also used elsewhere.
>

Fixed.

> > > The comment on heap_open() suggests that you could directly invoke
> > > relation_open() because you know this is a toast table, similarly for
> > > index_open(). (I can't say which is better style.)
> > >
> >
> > I don't know which is better style too... other opinions??
>
> Both are used several times in tablecmds.c, so both are probably fine.
> (Didn't check the contexts, though.)
>

Then we can leave that way. Is ok for you?

Greetings,

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

On Tue, Jul 22, 2014 at 3:29 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
> On Tue, Jul 22, 2014 at 12:01 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
> >
> >
> >
> > On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > >
> > > > That means should I "FlushRelationBuffers(rel)" before change the
> > > > relpersistence?
> > >
> > > I don't think that'd help. I think what this means that you simply
> > > cannot change the relpersistence of the old relation before the heap
> > > swap is successful. So I guess it has to be something like (pseudocode):
> > >
> > > OIDNewHeap = make_new_heap(..);
> > > newrel = heap_open(OIDNewHeap, AEL);
> > >
> > > /*
> > >  * Change the temporary relation to be unlogged/logged. We have to do
> > >  * that here so buffers for the new relfilenode will have the right
> > >  * persistency set while the original filenode's buffers won't get read
> > >  * in with the wrong (i.e. new) persistency setting. Otherwise a
> > >  * rollback after the rewrite would possibly result with buffers for the
> > >  * original filenode having the wrong persistency setting.
> > >  *
> > >  * NB: This relies on swap_relation_files() also swapping the
> > >  * persistency. That wouldn't work for pg_class, but that can't be
> > >  * unlogged anyway.
> > >  */
> > > AlterTableChangeCatalogToLoggedOrUnlogged(newrel);
> > > FlushRelationBuffers(newrel);
> > > /* copy heap data into newrel */
> > > finish_heap_swap();
> > >
> > > And then in swap_relation_files() also copy the persistency.
> > >
> > >
> > > That's the best I can come up right now at least.
> > >
> >
> > Isn't better if we can set the relpersistence as an argument to "make_new_heap" ?
> >
> > I'm thinking to change the make_new_heap:
> >
> > From:
> >
> >  make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
> >                LOCKMODE lockmode)
> >
> > To:
> >
> >  make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
> >                LOCKMODE lockmode)
> >
> > That way we can create the new heap with the appropriate relpersistence, so in the swap_relation_files also copy the persistency, of course.
> >
> > And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes, because in the "finish_heap_swap" they'll be rebuild.
> >
> > Thoughts?
> >
>
> The attached patch implement my previous idea based on Andres thoughts.
>

I don't liked the last version of the patch, especially this part:

+            /* check if SetUnlogged or SetLogged exists in subcmds */
+            for(pass = 0; pass < AT_NUM_PASSES; pass++)
+            {
+                List         *subcmds = tab->subcmds[pass];
+                ListCell    *lcmd;
+
+                if (subcmds == NIL)
+                    continue;
+
+                foreach(lcmd, subcmds)
+                {
+                    AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+
+                    if (cmd->subtype == AT_SetUnLogged || cmd->subtype == AT_SetLogged)
+                    {
+                        /*
+                         * Change the temporary relation to be unlogged/logged. We have to do
+                         * that here so buffers for the new relfilenode will have the right
+                         * persistency set while the original filenode's buffers won't get read
+                         * in with the wrong (i.e. new) persistency setting. Otherwise a
+                         * rollback after the rewrite would possibly result with buffers for the
+                         * original filenode having the wrong persistency setting.
+                         *
+                         * NB: This relies on swap_relation_files() also swapping the
+                         * persistency. That wouldn't work for pg_class, but that can't be
+                         * unlogged anyway.
+                         */
+                        if (cmd->subtype == AT_SetUnLogged)
+                            newrelpersistence = RELPERSISTENCE_UNLOGGED;
+
+                        isSetLoggedUnlogged = true;
+                    }
+                }
+            }

So I did a refactoring adding new items to AlteredTableInfo to pass the information through the phases.

Regards,

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

On Sat, Nov 15, 2014 at 2:23 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Michael Paquier wrote:
>
> > Btw, perhaps this diff should be pushed as a different patch as this is a
> > rather different thing:
> > -       if (heapRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED
> > &&
> > +       if (indexRelation->rd_rel->relpersistence ==
> > RELPERSISTENCE_UNLOGGED &&
> >                 !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM)
> > As this is a correctness fix, it does not seem necessary to back-patch it:
> > the parent relation always has the same persistence as its indexes.
>
> There was an argument for doing it this way that only applies if this
> patch went in, but I can't remember now what it was.
>
> Anyway I pushed the patch after some slight additional changes.  Thanks!
>

Thanks!

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Wed, Jul 23, 2014 at 5:48 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> On Tue, Jul 22, 2014 at 3:29 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
> >
> > On Tue, Jul 22, 2014 at 12:01 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
> > >
> > >
> > >
> > > On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > > >
> > > > > That means should I "FlushRelationBuffers(rel)" before change the
> > > > > relpersistence?
> > > >
> > > > I don't think that'd help. I think what this means that you simply
> > > > cannot change the relpersistence of the old relation before the heap
> > > > swap is successful. So I guess it has to be something like (pseudocode):
> > > >
> > > > OIDNewHeap = make_new_heap(..);
> > > > newrel = heap_open(OIDNewHeap, AEL);
> > > >
> > > > /*
> > > >  * Change the temporary relation to be unlogged/logged. We have to do
> > > >  * that here so buffers for the new relfilenode will have the right
> > > >  * persistency set while the original filenode's buffers won't get read
> > > >  * in with the wrong (i.e. new) persistency setting. Otherwise a
> > > >  * rollback after the rewrite would possibly result with buffers for the
> > > >  * original filenode having the wrong persistency setting.
> > > >  *
> > > >  * NB: This relies on swap_relation_files() also swapping the
> > > >  * persistency. That wouldn't work for pg_class, but that can't be
> > > >  * unlogged anyway.
> > > >  */
> > > > AlterTableChangeCatalogToLoggedOrUnlogged(newrel);
> > > > FlushRelationBuffers(newrel);
> > > > /* copy heap data into newrel */
> > > > finish_heap_swap();
> > > >
> > > > And then in swap_relation_files() also copy the persistency.
> > > >
> > > >
> > > > That's the best I can come up right now at least.
> > > >
> > >
> > > Isn't better if we can set the relpersistence as an argument to "make_new_heap" ?
> > >
> > > I'm thinking to change the make_new_heap:
> > >
> > > From:
> > >
> > >  make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
> > >                LOCKMODE lockmode)
> > >
> > > To:
> > >
> > >  make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
> > >                LOCKMODE lockmode)
> > >
> > > That way we can create the new heap with the appropriate relpersistence, so in the swap_relation_files also copy the persistency, of course.
> > >
> > > And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes, because in the "finish_heap_swap" they'll be rebuild.
> > >
> > > Thoughts?
> > >
> >
> > The attached patch implement my previous idea based on Andres thoughts.
> >
>
> I don't liked the last version of the patch, especially this part:
>
> +            /* check if SetUnlogged or SetLogged exists in subcmds */
> +            for(pass = 0; pass < AT_NUM_PASSES; pass++)
> +            {
> +                List         *subcmds = tab->subcmds[pass];
> +                ListCell    *lcmd;
> +
> +                if (subcmds == NIL)
> +                    continue;
> +
> +                foreach(lcmd, subcmds)
> +                {
> +                    AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
> +
> +                    if (cmd->subtype == AT_SetUnLogged || cmd->subtype == AT_SetLogged)
> +                    {
> +                        /*
> +                         * Change the temporary relation to be unlogged/logged. We have to do
> +                         * that here so buffers for the new relfilenode will have the right
> +                         * persistency set while the original filenode's buffers won't get read
> +                         * in with the wrong (i.e. new) persistency setting. Otherwise a
> +                         * rollback after the rewrite would possibly result with buffers for the
> +                         * original filenode having the wrong persistency setting.
> +                         *
> +                         * NB: This relies on swap_relation_files() also swapping the
> +                         * persistency. That wouldn't work for pg_class, but that can't be
> +                         * unlogged anyway.
> +                         */
> +                        if (cmd->subtype == AT_SetUnLogged)
> +                            newrelpersistence = RELPERSISTENCE_UNLOGGED;
> +
> +                        isSetLoggedUnlogged = true;
> +                    }
> +                }
> +            }
>
>
> So I did a refactoring adding new items to AlteredTableInfo to pass the information through the phases.
>

Hi all,

There are something that should I do on this patch yet?

Regards

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

On Fri, Aug 22, 2014 at 3:32 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Fabrízio de Royes Mello wrote:
> > Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera <
> > alvherre@2ndquadrant.com> escreveu:
> >
> > > Fabrízio de Royes Mello wrote:
> > >
> > > > I forgot to mention... I did again a lot of tests using different
> > > > replication scenarios to make sure all is ok:
> > > > - slaves async
> > > > - slaves sync
> > > > - cascade slaves
> > >
> > > On v13 you mean?
> > >
> > Exactly!
>
> Great.  Pushed.  Thanks for the patch.
>

Awesome!!! Actually I should say thank you my friend!! See you in Campinas...

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

>> Github: http://github.com/fabriziomello

On Wed, Jul 16, 2014 at 3:53 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>
> On 2014-07-16 20:25:42 +0200, Andres Freund wrote:
> > Hi,
> >
> > I quickly looked at this patch and I think there's major missing pieces
> > around buffer management and wal logging.
> >
> > a) Currently buffers that are in memory marked as
> >    permanent/non-permanent aren't forced out to disk/pruned from cache,
> >    not even when they're dirty.
> > b) When converting from a unlogged to a logged table the relation needs
> >    to be fsynced.
> > c) Currently a unlogged table changed into a logged one will be
> >    corrupted on a standby because its contents won't ever be WAL logged.
>
> Forget that, didn't notice that you're setting tab->rewrite = true.
>

:-)

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

On Wed, Aug 20, 2014 at 12:35 PM, Thom Brown <thom@linux.com> wrote:
>
> Hi Fabrizio,
>
> +      This form changes the table persistence type from unlogged to permanent or
> +      from unlogged to permanent (see <xref linkend="SQL-CREATETABLE-UNLOGGED">).
>
> Shouldn't this read "unlogged to permanent or from permanent to unlogged"?
>

Fixed.

> "ERROR:  table test is not permanent"
>
> Perhaps this would be better as "table test is unlogged" as "permanent" doesn't match the term used in the DDL syntax.
>

Fixed.

> Gave the patch a quick test-drive on a replicated instance, and it appears to operate as described.
>

Thanks for your review.

Att,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Mon, Jul 28, 2014 at 2:24 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> On Mon, Jul 28, 2014 at 1:41 PM, Christoph Berg <cb@df7cb.de> wrote:
> >
> > Re: Fabrízio de Royes Mello 2014-07-28 <CAFcNs+pctx4Q2UYsLOvVFWaznO3U0XhPpkMx5DRhR=Jw8w3tYg@mail.gmail.com>
> > > There are something that should I do on this patch yet?
> >
> > I haven't got around to have a look at the newest incarnation yet, but
> > I plan to do that soonish. (Of course that shouldn't stop others from
> > doing that as well if they wish.)
> >
>
> Thanks!
>

Updated version.

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

On Fri, Aug 22, 2014 at 4:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Aug 22, 2014 at 2:32 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Fabrízio de Royes Mello wrote:
> >> Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera <
> >> alvherre@2ndquadrant.com> escreveu:
> >>
> >> > Fabrízio de Royes Mello wrote:
> >> >
> >> > > I forgot to mention... I did again a lot of tests using different
> >> > > replication scenarios to make sure all is ok:
> >> > > - slaves async
> >> > > - slaves sync
> >> > > - cascade slaves
> >> >
> >> > On v13 you mean?
> >> >
> >> Exactly!
> >
> > Great.  Pushed.  Thanks for the patch.
>
> Hmm.  I confess to not having paid enough attention to this, but:
>
> 1. Loggedness is not a word.  I think that "persistence" or
> "relpersistence" would be better.
>

> 2. The patch seems to think that it can sometimes be safe to change
> the relpersistence of an existing relation.  Unless you can be sure
> that no buffers can possibly be present in shared_buffers and nobody
> will use an existing relcache entry to read a new one in, it's not,
> because the buffers won't have the right BM_PERSISTENT marking.  I'm
> very nervous about the fact that this patch seems not to have touched
> bufmgr.c, but maybe I'm missing something.
>

Because of this concern I implement a solution pointed by Andres [1].

Regards,

[1] http://www.postgresql.org/message-id/20140717230220.GK21370@awork2.anarazel.de

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Tue, Jul 22, 2014 at 12:01 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
>
> On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >
> > > That means should I "FlushRelationBuffers(rel)" before change the
> > > relpersistence?
> >
> > I don't think that'd help. I think what this means that you simply
> > cannot change the relpersistence of the old relation before the heap
> > swap is successful. So I guess it has to be something like (pseudocode):
> >
> > OIDNewHeap = make_new_heap(..);
> > newrel = heap_open(OIDNewHeap, AEL);
> >
> > /*
> >  * Change the temporary relation to be unlogged/logged. We have to do
> >  * that here so buffers for the new relfilenode will have the right
> >  * persistency set while the original filenode's buffers won't get read
> >  * in with the wrong (i.e. new) persistency setting. Otherwise a
> >  * rollback after the rewrite would possibly result with buffers for the
> >  * original filenode having the wrong persistency setting.
> >  *
> >  * NB: This relies on swap_relation_files() also swapping the
> >  * persistency. That wouldn't work for pg_class, but that can't be
> >  * unlogged anyway.
> >  */
> > AlterTableChangeCatalogToLoggedOrUnlogged(newrel);
> > FlushRelationBuffers(newrel);
> > /* copy heap data into newrel */
> > finish_heap_swap();
> >
> > And then in swap_relation_files() also copy the persistency.
> >
> >
> > That's the best I can come up right now at least.
> >
>
> Isn't better if we can set the relpersistence as an argument to "make_new_heap" ?
>
> I'm thinking to change the make_new_heap:
>
> From:
>
>  make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
>                LOCKMODE lockmode)
>
> To:
>
>  make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
>                LOCKMODE lockmode)
>
> That way we can create the new heap with the appropriate relpersistence, so in the swap_relation_files also copy the persistency, of course.
>
> And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes, because in the "finish_heap_swap" they'll be rebuild.
>
> Thoughts?
>

The attached patch implement my previous idea based on Andres thoughts.

Regards,

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

On Tue, Aug 26, 2014 at 1:42 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> On Fri, Aug 22, 2014 at 5:23 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> >
> > Fabrízio de Royes Mello wrote:
> > > On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> > > wrote:
> >
> > > > I pointed out, in the email just before pushing the patch, that perhaps
> > > > we should pass down the new relpersistence flag into finish_heap_swap,
> > > > and from there we could pass it down to reindex_index() which is where
> > > > it would be needed.  I'm not sure it's worth the trouble, but I think we
> > > > can still ask Fabrizio to rework that part.
> >
> > > I can rework this part if it's a real concern.
> >
> > I guess we can make a better assessment by seeing what it would take.
> > I'm afraid it will turn out to be really ugly.
> >
> > Now, there's some desire to have unlogged indexes on logged tables; I
> > guess if we have that, then eventually there will also be a desire to
> > swap individual indexes from logged to unlogged.  Perhaps whatever fix
> > we come up with here would pave the road for that future feature.
> >
>
> Álvaro,
>
> I did a refactoring to pass down the relpersistence to "finish_heap_swap" and then change the catalog inside the "reindex_index" instead of in the rewrite table phase.
>
> That way we can get rid the function ATChangeIndexesPersistence in the src/backend/commands/tablecmds.c.
>
> Thoughts?
>

Patch rebased and added to commitfest [1].

Regards,

[1] https://commitfest.postgresql.org/action/commitfest_view?id=24

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Tue, Jul 15, 2014 at 3:04 PM, Christoph Berg <cb@df7cb.de> wrote:
>
> Hi Fabrízio,
>
> thanks for the speedy new version.
>

You're welcome... If all happen ok I would like to have this patch commited before the GSoC2014 ends.

> > > I've just tried some SET (UN)LOGGED operations with altering column
> > > types in the same operation, that works. But:
> > >
> > > Yes, you should use the existing table rewriting machinery, or at
> > > least clearly document (in comments) why it doesn't work for you.
> > >
> > > Also looking at ATController, there's a wqueue mechanism to queue
> > > catalog updates. You should probably use this, too, or again document
> > > why it doesn't work for you.
> > >
> >
> > This works... fixed!
>
> Thanks.
>
> 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.

> > > You miss the symmetric case the other way round. When changing a table
> > > to unlogged, you need to make sure no other permanent table is
> > > referencing our table.
> > >
> >
> > Ohh yeas... sorry... you're completely correct... fixed!
>
> Can you move ATPrepSetUnLogged next to ATPrepSetLogged as both
> reference AlterTableSetLoggedCheckForeignConstraints now, and fix the
> comment on ATPrepSetUnLogged to also mention FKs? I'd also reiterate
> my proposal to merge these into one function, given they are now doing
> the same checks.
>

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

> In AlterTableSetLoggedCheckForeignConstraints, move "relfk =
> relation_open..." out of the "if" because it's duplicated, and also for
> symmetry with relation_close().
>

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

> The function needs comments. It is somewhat clear that
> self-referencing FKs will be skipped, but the two "if (toLogged)"
> branches should be annotated to say which case they are really about.
>

Fixed.

> Instead of just switching the argument order in the errmsg arguments,
> the error text should be updated to read "table %s is referenced
> by permanent table %s". At the moment the error text is wrong because
> the table logged1 is not yet unlogged:
>
> +ALTER TABLE logged1 SET UNLOGGED;
> +ERROR:  table logged2 references unlogged table logged1
>
> -> table logged1 is referenced by permanent table logged2
>

Sorry... my mistake... fixed

> Compared to v3, you've removed a lot of "SELECT t.relpersistence..."
> from the regression tests, was that intended?
>

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

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

Regards,

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

--

On Thu, Nov 6, 2014 at 3:42 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On Sat, Sep 13, 2014 at 11:02 PM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > Patch rebased and added to commitfest [1].
> It looks like a good thing to remove ATChangeIndexesPersistence, this
> puts the persistence switch directly into reindex process.
>
> A couple of minor comments about this patch:
> 1) Reading it, I am wondering if it would not be finally time to
> switch to a macro to get a relation's persistence, something like
> RelationGetPersistence in rel.h... Not related directly to this patch.

Good idea... I'll provide a patch soon.

> 2) reindex_index has as new argument a relpersislence value for the
> new index. reindex_relation has differently a new set of flags to
> enforce the relpersistence of all the underling indexes. Wouldn't it
> be better for API consistency to pass directly a relpersistence value
> through reindex_relation? In any case, the comment block of
> reindex_relation does not contain a description of the new flags.

I did it because the ReindexDatabase build a list of oids to run reindex_relation for each item of the list. I can change the list to store the relpersistence also, but this can lead us to a concurrency trouble because if one session execute REINDEX DATABASE and other session run "ALTER TABLE name SET {LOGGED|UNLOGGED}" during the building of the list the reindex can lead us to a inconsitence state.

Added comments to comment block of reindex_relation.

> 3) Here you may as well just set the value and be done:
> +        /*
> +         * Check if need to set the new relpersistence
> +         */
> +        if (iRel->rd_rel->relpersistence != relpersistence)
> +            iRel->rd_rel->relpersistence = relpersistence;

Hmmm... I really don't know why I did it... fixed.

Thanks!

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Mon, Jul 28, 2014 at 1:41 PM, Christoph Berg <cb@df7cb.de> wrote:
>
> Re: Fabrízio de Royes Mello 2014-07-28 <CAFcNs+pctx4Q2UYsLOvVFWaznO3U0XhPpkMx5DRhR=Jw8w3tYg@mail.gmail.com>
> > There are something that should I do on this patch yet?
>
> I haven't got around to have a look at the newest incarnation yet, but
> I plan to do that soonish. (Of course that shouldn't stop others from
> doing that as well if they wish.)
>

Thanks!

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

On Tue, Jul 8, 2014 at 4:53 PM, Christoph Berg <cb@df7cb.de> wrote:
>
> Hi,
>
> here's my review for this patch.
>
> Generally, the patch addresses a real need, tables often only turn
> out to be desired as unlogged if there are performance problems in
> practice, and the other way round changing an unlogged table to logged
> is way more involved manually than it could be with this patch. So
> yes, we want the feature.
>
> I've tried the patch, and it works as desired, including tab
> completion in psql. There are a few issues to be resolved, though.
>

Thank you so much for your review!

> Re: Fabrízio de Royes Mello 2014-06-26 <CAFcNs+pyV0PBjLo97OSyqV1yQOC7s+JGvWXc8UnY5jSRDwy45A@mail.gmail.com>
> > As part of GSoC2014 and with agreement of my mentor and reviewer (Stephen
> > Frost) I've send a complement of the first patch to add the capability to
> > change a regular table to unlogged.
>
> (Fwiw, I believe this direction is the more interesting one.)
>

:-)


> > With this patch we finish the main goals of the GSoC2014 and now we'll work
> > in the additional goals.
>
> ... that being the non-WAL-logging with wal_level=minimal, or more?
>

This is the first of additional goals,  but we have others. Please see [1].

> > diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
> > index 69a1e14..424f2e9 100644
> > --- a/doc/src/sgml/ref/alter_table.sgml
> > +++ b/doc/src/sgml/ref/alter_table.sgml
> > @@ -58,6 +58,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
> >      ENABLE REPLICA RULE <replaceable class="PARAMETER">rewrite_rule_name</replaceable>
> >      ENABLE ALWAYS RULE <replaceable class="PARAMETER">rewrite_rule_name</replaceable>
> >      CLUSTER ON <replaceable class="PARAMETER">index_name</replaceable>
> > +    SET {LOGGED | UNLOGGED}
> >      SET WITHOUT CLUSTER
> >      SET WITH OIDS
> >      SET WITHOUT OIDS
>
> This must not be between the two CLUSTER lines. I think the best spot
> would just be one line down, before SET WITH OIDS.
>

Fixed.

> (While we are at it, SET TABLESPACE should probably be moved up to the
> other SET options...)
>

Fixed.

> > @@ -432,6 +433,20 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
> >     </varlistentry>
> >
> >     <varlistentry>
> > +    <term><literal>SET {LOGGED | UNLOGGED}</literal></term>
> > +    <listitem>
> > +     <para>
> > +      This form change the table persistence type from unlogged to permanent or
>
> This grammar bug pops up consistently: This form *changes*...
>
> There's trailing whitespace.
>

Fixed.

> > +      from unlogged to permanent by rewriting the entire contents of the table
> > +      and associated indexes into new disk files.
> > +     </para>
> > +     <para>
> > +      Changing the table persistence type acquires an <literal>ACCESS EXCLUSIVE</literal> lock.
> > +     </para>
>
> I'd rewrite that and add a reference:
>
> ... from unlogged to permanent (see <xref linkend="SQL-CREATETABLE-UNLOGGED">).
> </para>
> <para>
> Changing the table persistence type acquires an <literal>ACCESS EXCLUSIVE</literal> lock
> and rewrites the table contents and associated indexes into new disk files.
> </para>
>

Fixed.

> > diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> > index 60d387a..9dfdca2 100644
> > --- a/src/backend/commands/tablecmds.c
> > +++ b/src/backend/commands/tablecmds.c
> > @@ -125,18 +125,19 @@ static List *on_commits = NIL;
> >   * a pass determined by subcommand type.
> >   */
> >
> > -#define AT_PASS_UNSET                        -1              /* UNSET will cause ERROR */
> > -#define AT_PASS_DROP                 0               /* DROP (all flavors) */
> > -#define AT_PASS_ALTER_TYPE           1               /* ALTER COLUMN TYPE */
> > -#define AT_PASS_OLD_INDEX            2               /* re-add existing indexes */
> > -#define AT_PASS_OLD_CONSTR           3               /* re-add existing constraints */
> > -#define AT_PASS_COL_ATTRS            4               /* set other column attributes */
> > +#define AT_PASS_UNSET                                -1              /* UNSET will cause ERROR */
> > +#define AT_PASS_DROP                         0               /* DROP (all flavors) */
> > +#define AT_PASS_ALTER_TYPE                   1               /* ALTER COLUMN TYPE */
> > +#define AT_PASS_OLD_INDEX                    2               /* re-add existing indexes */
> > +#define AT_PASS_OLD_CONSTR                   3               /* re-add existing constraints */
> > +#define AT_PASS_COL_ATTRS                    4               /* set other column attributes */
> >  /* We could support a RENAME COLUMN pass here, but not currently used */
> > -#define AT_PASS_ADD_COL                      5               /* ADD COLUMN */
> > -#define AT_PASS_ADD_INDEX            6               /* ADD indexes */
> > -#define AT_PASS_ADD_CONSTR           7               /* ADD constraints, defaults */
> > -#define AT_PASS_MISC                 8               /* other stuff */
> > -#define AT_NUM_PASSES                        9
> > +#define AT_PASS_ADD_COL                              5               /* ADD COLUMN */
> > +#define AT_PASS_ADD_INDEX                    6               /* ADD indexes */
> > +#define AT_PASS_ADD_CONSTR                   7               /* ADD constraints, defaults */
> > +#define AT_PASS_MISC                         8               /* other stuff */
> > +#define AT_PASS_SET_LOGGED_UNLOGGED  9               /* SET LOGGED and UNLOGGED */
> > +#define AT_NUM_PASSES                                10
>
>
> This unnecessarily rewrites all the tabs, but see below.
>

I did that because the new constant AT_PASS_SET_LOGGED_UNLOGGED is larger than others.

> > @@ -376,6 +377,7 @@ static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMOD
> >  static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
> >                                        char *cmd, List **wqueue, LOCKMODE lockmode,
> >                                        bool rewrite);
> > +static void ATPostAlterSetLoggedUnlogged(Oid relid);
>
> (See below)
>
> >  static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
> >  static void TryReuseForeignKey(Oid oldId, Constraint *con);
> >  static void change_owner_fix_column_acls(Oid relationOid,
> > @@ -402,6 +404,13 @@ static void ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockm
> >  static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
> >  static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode);
> >  static void ATExecGenericOptions(Relation rel, List *options);
> > +static void ATPrepSetLogged(Relation rel);
> > +static void ATPrepSetUnLogged(Relation rel);
> > +static void ATExecSetLogged(Relation rel);
> > +static void ATExecSetUnLogged(Relation rel);
> > +
> > +static void AlterTableSetLoggedCheckForeignConstraints(Relation rel);
> > +static void AlterTableSetLoggedOrUnlogged(Relation rel, bool toLogged);
>
> All that should be ordered like in the docs, i.e. after
> ATExecDropCluster. (... and the SetTableSpace stuff is already here ...)
>

Fixed.

> > +             case AT_SetLogged:
> > +             case AT_SetUnLogged:
> > +                     ATSimplePermissions(rel, ATT_TABLE);
> > +                     if (cmd->subtype == AT_SetLogged)
> > +                             ATPrepSetLogged(rel);                   /* SET LOGGED */
> > +                     else
> > +                             ATPrepSetUnLogged(rel);                 /* SET UNLOGGED */
> > +                     pass = AT_PASS_SET_LOGGED_UNLOGGED;
> > +                     break;
>
> I'm wondering if you shouldn't make a single ATPrepSetLogged function
> that takes and additional toLogged argument. Or alternatively get rid
> of the if() by putting the code also into case AT_SetLogged.
>

Actually I started that way... with just one ATPrep function we have some additional complexity to check relpersistence, define the error message and to run AlterTableSetLoggedCheckForeignConstraints(rel) function. So to simplify the code I decided split in two small functions.

> > @@ -3307,6 +3327,9 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
> >                               ATPostAlterTypeCleanup(wqueue, tab, lockmode);
> >
> >                       relation_close(rel, NoLock);
> > +
> > +                     if (pass == AT_PASS_SET_LOGGED_UNLOGGED)
> > +                             ATPostAlterSetLoggedUnlogged(RelationGetRelid(rel));
>
> This must be done before relation_close() which releases all locks.
>
> Moreover, I think you can get rid of that extra PASS here.
> AT_PASS_ALTER_TYPE has its own pass because you can alter several
> columns in a single ALTER TABLE statement, but you can have only one
> SET (UN)LOGGED, so you can to the cluster_rel() directly in
> AlterTableSetLoggedOrUnlogged() (unless cluster_rel() is too intrusive
> and would interfere with other ALTER TABLE operations in this command,
> no idea).
>

I had some troubles here so I decided to do in that way, but I confess I'm not comfortable with this implementation. Looking more carefully on tablecmds.c code, at the ATController we have three phases and the third is 'scan/rewrite tables as needed' so my doubt is if can I use it instead of call 'cluster_rel'?

> > @@ -3526,6 +3549,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
> >               case AT_GenericOptions:
> >                       ATExecGenericOptions(rel, (List *) cmd->def);
> >                       break;
> > +             case AT_SetLogged:
> > +                     ATExecSetLogged(rel);
> > +                     break;
> > +             case AT_SetUnLogged:
> > +                     ATExecSetUnLogged(rel);
> > +                     break;
>
> I'd replace ATExecSetLogged and ATExecSetUnLogged directly with
> AlterTableSetLoggedOrUnlogged(rel, true/false). The 1-line wrappers
> don't buy you anything.
>

Fixed.

> > +static void
> > +AlterTableSetLoggedCheckForeignConstraints(Relation rel)
> [...]
>
> I can't comment on the quality of this function, but it seems to be
> doing its job.
>

:-)


> > +/*
> > + * ALTER TABLE <name> SET UNLOGGED
> > + *
> > + * Change the table persistence type from permanent to unlogged by
> > + * rewriting the entire contents of the table and associated indexes
> > + * into new disk files.
> > + *
> > + * The ATPrepSetUnLogged function check all precondictions to perform
> > + * the operation:
> > + * - check if the target table is permanent
> > + */
> > +static void
> > +ATPrepSetUnLogged(Relation rel)
> > +{
> > +     /* check if is an permanent relation */
> > +     if (rel->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT)
> > +             ereport(ERROR,
> > +                             (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> > +                              errmsg("table %s is not permanent",
> > +                              RelationGetRelationName(rel))));
> > +}
>
> Here's the big gotcha: Just like SET LOGGED must check for outgoing
> FKs to unlogged tables, SET UNLOGGED must check for incoming FKs from
> permanent tables. This is missing.
>

I don't think so... we can create an unlogged table with a FK referring to a regular table...

fabrizio=# create table foo(id integer primary key);
CREATE TABLE
fabrizio=# create unlogged table bar(id integer primary key, foo integer references foo);
CREATE TABLE

... but is not possible create a FK from a regular table referring to an unlogged table:

fabrizio=# create unlogged table foo(id integer primary key);
CREATE TABLE
fabrizio=# create table bar(id integer primary key, foo integer references foo);
ERROR:  constraints on permanent tables may reference only permanent tables

... and a FK from an unlogged table referring other unlogged table works:

fabrizio=# create unlogged table bar(id integer primary key, foo integer references foo);
CREATE TABLE

So we must take carefull just when changing an unlogged table to a regular table.

Am I correct or I miss something?

> > +/*
> > + * The AlterTableChangeCatalogToLoggedOrUnlogged function perform the
> > + * catalog changes, i.e. update pg_class.relpersistence to 'p' or 'u'
> > + */
> > +static void
> > +AlterTableChangeCatalogToLoggedOrUnlogged(Relation rel, Relation relrelation, bool toLogged)
> > +{
> > +     HeapTuple               tuple;
> > +     Form_pg_class   pg_class_form;
> > +
> > +     tuple = SearchSysCacheCopy1(RELOID,
> > +                                                              ObjectIdGetDatum(RelationGetRelid(rel)));
> > +     if (!HeapTupleIsValid(tuple))
> > +             elog(ERROR, "cache lookup failed for relation %u",
> > +                      RelationGetRelid(rel));
> > +
> > +     pg_class_form = (Form_pg_class) GETSTRUCT(tuple);
> > +
> > +     Assert(pg_class_form->relpersistence ==
> > +             ((toLogged) ? RELPERSISTENCE_UNLOGGED : RELPERSISTENCE_PERMANENT));
> > +
> > +     pg_class_form->relpersistence = toLogged ?
> > +                     RELPERSISTENCE_PERMANENT : RELPERSISTENCE_UNLOGGED;
> > +
> > +     simple_heap_update(relrelation, &tuple->t_self, tuple);
> > +
> > +     /* keep catalog indexes current */
> > +     CatalogUpdateIndexes(relrelation, tuple);
> > +
> > +     heap_freetuple(tuple);
> > +}
>
> Again I can't comment on the low-level catalog stuff - though I'd
> remove some of those blank lines.
>

Fixed.

> > +/*
> > + * The AlterTableSetLoggedOrUnlogged function contains the main logic
> > + * of the operation, changing the catalog for main heap, toast and indexes
> > + */
> > +static void
> > +AlterTableSetLoggedOrUnlogged(Relation rel, bool toLogged)
> > +{
> > +     Oid                     relid;
> > +     Relation        indexRelation;
> > +     ScanKeyData skey;
> > +     SysScanDesc scan;
> > +     HeapTuple       indexTuple;
> > +     Relation        relrel;
> > +
> > +     relid = RelationGetRelid(rel);
> > +
> > +     /* open pg_class to update relpersistence */
> > +     relrel = heap_open(RelationRelationId, RowExclusiveLock);
> > +
> > +     /* main heap */
> > +     AlterTableChangeCatalogToLoggedOrUnlogged(rel, relrel, toLogged);
> > +
> > +     /* toast heap, if any */
> > +     if (OidIsValid(rel->rd_rel->reltoastrelid))
> > +     {
> > +             Relation        toastrel;
> > +
> > +             toastrel = heap_open(rel->rd_rel->reltoastrelid, AccessShareLock);
> > +             AlterTableChangeCatalogToLoggedOrUnlogged(toastrel, relrel, toLogged);
> > +             heap_close(toastrel, AccessShareLock);
>
> The comment on heap_open() suggests that you could directly invoke
> relation_open() because you know this is a toast table, similarly for
> index_open(). (I can't say which is better style.)
>

I don't know which is better style too... other opinions??

> > +     }
> > +
> > +     /* Prepare to scan pg_index for entries having indrelid = this rel. */
> > +     indexRelation = heap_open(IndexRelationId, AccessShareLock);
> > +     ScanKeyInit(&skey,
> > +                             Anum_pg_index_indrelid,
> > +                             BTEqualStrategyNumber, F_OIDEQ,
> > +                             relid);
> > +
> > +     scan = systable_beginscan(indexRelation, IndexIndrelidIndexId, true,
> > +                                                       NULL, 1, &skey);
> > +
> > +     while (HeapTupleIsValid(indexTuple = systable_getnext(scan)))
> > +     {
> > +             Form_pg_index index = (Form_pg_index) GETSTRUCT(indexTuple);
> > +             Relation indxrel = index_open(index->indexrelid, AccessShareLock);
> > +
> > +             AlterTableChangeCatalogToLoggedOrUnlogged(indxrel, relrel, toLogged);
> > +
> > +             index_close(indxrel, AccessShareLock);
> > +     }
>
> You forgot the TOAST index.
>

Fixed.

> > diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> > index 605c9b4..a784d73 100644
> > --- a/src/backend/parser/gram.y
> > +++ b/src/backend/parser/gram.y
>
> > @@ -2201,6 +2201,20 @@ alter_table_cmd:
> >                                       n->def = $3;
> >                                       $ = (Node *)n;
> >                               }
> > +                     /* ALTER TABLE <name> SET LOGGED  */
> > +                     | SET LOGGED
> > +                             {
> > +                                     AlterTableCmd *n = makeNode(AlterTableCmd);
> > +                                     n->subtype = AT_SetLogged;
> > +                                     $ = (Node *)n;
> > +                             }
> > +                     /* ALTER TABLE <name> SET UNLOGGED  */
> > +                     | SET UNLOGGED
> > +                             {
> > +                                     AlterTableCmd *n = makeNode(AlterTableCmd);
> > +                                     n->subtype = AT_SetUnLogged;
> > +                                     $ = (Node *)n;
> > +                             }
>
> Also move up to match the docs/other code ordering.
>

Fixed. But other ALTER commands in gram.y aren't in the same order of documentation.

> > index ff126eb..dc9f8fa 100644
> > --- a/src/include/nodes/parsenodes.h
> > +++ b/src/include/nodes/parsenodes.h
> > @@ -1331,7 +1331,9 @@ typedef enum AlterTableType
> >       AT_AddOf,                                       /* OF <type_name> */
> >       AT_DropOf,                                      /* NOT OF */
> >       AT_ReplicaIdentity,                     /* REPLICA IDENTITY */
> > -     AT_GenericOptions                       /* OPTIONS (...) */
> > +     AT_GenericOptions,                      /* OPTIONS (...) */
> > +     AT_SetLogged,                           /* SET LOGGED */
> > +     AT_SetUnLogged                          /* SET UNLOGGED */
>
> Likewise.
>

Fixed.

> > --- a/src/test/regress/expected/alter_table.out
> > +++ b/src/test/regress/expected/alter_table.out
>
> Please add test cases for incoming FKs, TOAST table relpersistence,
> and TOAST index relpersistence.
>

Added.

> Thanks for working on this feature, I'm looking forward to it!
>

You're welcome!

Regards,

[1] https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014

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

On Wed, Jul 16, 2014 at 7:26 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>
> On 2014-07-16 20:53:06 +0200, Andres Freund wrote:
> > On 2014-07-16 20:25:42 +0200, Andres Freund wrote:
> > > Hi,
> > >
> > > I quickly looked at this patch and I think there's major missing pieces
> > > around buffer management and wal logging.
> > >
> > > a) Currently buffers that are in memory marked as
> > >    permanent/non-permanent aren't forced out to disk/pruned from cache,
> > >    not even when they're dirty.
> > > b) When converting from a unlogged to a logged table the relation needs
> > >    to be fsynced.
> > > c) Currently a unlogged table changed into a logged one will be
> > >    corrupted on a standby because its contents won't ever be WAL logged.
> >
> > Forget that, didn't notice that you're setting tab->rewrite = true.
>
> So, while that danger luckily isn't there I think there's something
> similar. Consider:
>
> CREATE TABLE blub(...);
> INSERT INTO blub ...;
>
> BEGIN;
> ALTER TABLE blub SET UNLOGGED;
> ROLLBACK;
>
> The rewrite will read in the 'old' contents - but because it's done
> after the pg_class.relpersistence is changed they'll all not be marked
> as BM_PERMANENT in memory. Then the ALTER TABLE is rolled back,
> including the relpersistence setting. Which will unfortunately leave
> pages with the wrong persistency setting in memory, right?
>

Regards,

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

On Fri, Aug 22, 2014 at 5:23 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Fabrízio de Royes Mello wrote:
> > On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> > wrote:
>
> > > I pointed out, in the email just before pushing the patch, that perhaps
> > > we should pass down the new relpersistence flag into finish_heap_swap,
> > > and from there we could pass it down to reindex_index() which is where
> > > it would be needed.  I'm not sure it's worth the trouble, but I think we
> > > can still ask Fabrizio to rework that part.
>
> > I can rework this part if it's a real concern.
>
> I guess we can make a better assessment by seeing what it would take.
> I'm afraid it will turn out to be really ugly.
>
> Now, there's some desire to have unlogged indexes on logged tables; I
> guess if we have that, then eventually there will also be a desire to
> swap individual indexes from logged to unlogged.  Perhaps whatever fix
> we come up with here would pave the road for that future feature.
>

Álvaro,

I did a refactoring to pass down the relpersistence to "finish_heap_swap" and then change the catalog inside the "reindex_index" instead of in the rewrite table phase.

That way we can get rid the function ATChangeIndexesPersistence in the src/backend/commands/tablecmds.c.

Thoughts?

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

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

On Mon, Aug 25, 2014 at 2:55 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Fabrízio de Royes Mello wrote:
> > On Sat, Aug 23, 2014 at 8:53 AM, Michael Paquier <michael.paquier@gmail.com>
> > wrote:
> > >
> > > On Sat, Aug 23, 2014 at 3:32 AM, Alvaro Herrera
> > > <alvherre@2ndquadrant.com> wrote:
> > > > Great.  Pushed.  Thanks for the patch.
> > > There is a typo in what has been pushed. Patch attached.
> > >
> >
> > Thanks... I fixed that in my last patch to change 'loggedness' to
> > 'persistence'. Attached.
>
> Thanks, pushed.  I also added a comment explaining why it's okay to do
> what we're doing.
>

Thanks...

I'm working on a refactoring to pass down the relpersistence flag to finish_heap_swap... is this valid for now or I should leave it to another patch?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello