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

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.

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?


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

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

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

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


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

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


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

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

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

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

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

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

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

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

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

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


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

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

Re: Fabrízio de Royes Mello 2014-07-12 <CAFcNs+qF5fUkkp9vdJWokiaBn_rRM4+HJqXeeVpD_7-tO0L4AA@mail.gmail.com>
> > ... 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].

Oh I wasn't aware of the wiki page, I had just read the old thread.
Thanks for the pointer.

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

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

Ah, ok then.

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

Nod.

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

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

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

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

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

Christoph
--
cb@df7cb.de | http://www.df7cb.de/

Hi Fabrízio,

thanks for the speedy new version.

Re: Fabrízio de Royes Mello 2014-07-15 <CAFcNs+opBuHjUo1sOATHv8zqcOwqp-yeRjFoo15v1xPXSpCdDw@mail.gmail.com>
> > > > > +
> > > > > +                     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

Oh there was one "not" too much for me, sorry. Anyway, this isn't
relevant anymore. :)

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

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

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

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.

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

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

Yes. It was just a minor nitpick anyway.


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

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

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

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.

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

Thanks.

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

Oh sorry. I had looked for that, but still missed it.

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

Nod.

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

Thanks, looks nice now.

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

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

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

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

permanent

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

if no ... exists

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

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

> + *
> + * Throws an exception when:

"when: if" is duplicated.

Throws exceptions:

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

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

Self-referencing foreign keys ...

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

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

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

...

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

...

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

after changing to permament

> +SELECT relname, relpersistence, oid = relfilenode AS original_relfilenode FROM pg_class WHERE relname ~ '^unlogged1'
ORDERBY 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

> +SELECT relname, relpersistence, oid = relfilenode AS original_relfilenode FROM pg_class WHERE relname ~ '^logged1'
ORDERBY 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 

...

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

> +SELECT relname, relpersistence, oid = relfilenode AS original_relfilenode FROM pg_class WHERE relname ~ '^logged1'
ORDERBY 1;
 
> +DROP TABLE logged3;
> +DROP TABLE logged2;
> +DROP TABLE logged1;


I think we are almost there :)

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

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

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

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.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

<div dir="ltr"><div class="gmail_extra"><br />On Wed, Jul 16, 2014 at 3:53 PM, Andres Freund <<a
href="mailto:andres@2ndquadrant.com">andres@2ndquadrant.com</a>>wrote:<br />><br />> On 2014-07-16 20:25:42
+0200,Andres Freund wrote:<br /> > > Hi,<br />> ><br />> > I quickly looked at this patch and I think
there'smajor missing pieces<br />> > around buffer management and wal logging.<br />> ><br />> > a)
Currentlybuffers that are in memory marked as<br /> > >    permanent/non-permanent aren't forced out to
disk/prunedfrom cache,<br />> >    not even when they're dirty.<br />> > b) When converting from a unlogged
toa logged table the relation needs<br /> > >    to be fsynced.<br />> > c) Currently a unlogged table
changedinto a logged one will be<br />> >    corrupted on a standby because its contents won't ever be WAL
logged.<br/>><br />> Forget that, didn't notice that you're setting tab->rewrite = true.<br /> ><br /><br
/>:-)<br/><br />--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

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?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 2014-07-16 20:45:15 -0300, Fabrízio de Royes Mello wrote:
> 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?
> >
> 
> 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
willhave the right* persistency set while the original filenode's buffers won't get read* in with the wrong (i.e. new)
persistencysetting. Otherwise a* rollback after the rewrite would possibly result with buffers for the* original
filenodehaving 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.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Re: Fabrízio de Royes Mello 2014-07-16 <CAFcNs+r_LMDDxJrAbWNJ3rMY0Qegwa-vv6V2SmDescOfb2dj+Q@mail.gmail.com>
> Anyway I think all is ok now. Is this ok for you?

Hi Fabrízio,

it's ok for me now, though Andres' concerns seem valid.

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

I meant to say that when this patch gets committed, the commit message
might want to explain that while we are at editing this doc part, we
moved SET TABLESPACE to the place where it really should be.

> > I think we are almost there :)
> 
> Yeah... thanks a lot for your help.

Welcome. I'll look forward to use this in production :)

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

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?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

<div dir="ltr"><div class="gmail_extra"><br />On Mon, Jul 21, 2014 at 9:51 AM, Andres Freund <<a
href="mailto:andres@2ndquadrant.com">andres@2ndquadrant.com</a>>wrote:<br />><br />> On 2014-07-16 20:45:15
-0300,Fabrízio de Royes Mello wrote:<br /> > > > The rewrite will read in the 'old' contents - but because
it'sdone<br />> > > after the pg_class.relpersistence is changed they'll all not be marked<br />> > >
asBM_PERMANENT in memory. Then the ALTER TABLE is rolled back,<br /> > > > including the relpersistence
setting.Which will unfortunately leave<br />> > > pages with the wrong persistency setting in memory,
right?<br/>> > ><br />> ><br />> > That means should I "FlushRelationBuffers(rel)" before change
the<br/> > > relpersistence?<br />><br />> Did my explanation clarify the problem + possible solution
sufficiently?<br/>><br /><br />Yes, your explanation was enough. I didn't had time to working on it yet. But I hope 
workingon it today or tomorrow at least.<br /><br /></div><div class="gmail_extra">Regards,<br /><br /></div><div
class="gmail_extra">--<br/>Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/> >> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/>>> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/> >> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

<div dir="ltr"><div class="gmail_extra"><br /><br />On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund <<a
href="mailto:andres@2ndquadrant.com"target="_blank">andres@2ndquadrant.com</a>> wrote:<br />><br />> > That
meansshould I "FlushRelationBuffers(rel)" before change the<br /> > > relpersistence?<br />><br />> I don't
thinkthat'd help. I think what this means that you simply<br />> cannot change the relpersistence of the old
relationbefore the heap<br />> swap is successful. So I guess it has to be something like (pseudocode):<br />
><br/>> OIDNewHeap = make_new_heap(..);<br />> newrel = heap_open(OIDNewHeap, AEL);<br />><br />> /*<br
/>> * Change the temporary relation to be unlogged/logged. We have to do<br />>  * that here so buffers for the
newrelfilenode will have the right<br /> >  * persistency set while the original filenode's buffers won't get
read<br/>>  * in with the wrong (i.e. new) persistency setting. Otherwise a<br />>  * rollback after the rewrite
wouldpossibly result with buffers for the<br /> >  * original filenode having the wrong persistency setting.<br
/>> *<br />>  * NB: This relies on swap_relation_files() also swapping the<br />>  * persistency. That
wouldn'twork for pg_class, but that can't be<br /> >  * unlogged anyway.<br />>  */<br />>
AlterTableChangeCatalogToLoggedOrUnlogged(newrel);<br/>> FlushRelationBuffers(newrel);<br />> /* copy heap data
intonewrel */<br />> finish_heap_swap();<br />><br />> And then in swap_relation_files() also copy the
persistency.<br/> ><br />><br />> That's the best I can come up right now at least.<br />><br /><br
/></div><divclass="gmail_extra">Isn't better if we can set the relpersistence as an argument to "make_new_heap" ?<br
/><br/></div><div class="gmail_extra">I'm thinking to change the make_new_heap:<br /></div><div class="gmail_extra"><br
/></div><divclass="gmail_extra">From: <br /></div><div class="gmail_extra"><br /> make_new_heap(Oid OIDOldHeap, Oid
NewTableSpace,bool forcetemp,<br />                LOCKMODE lockmode)<br /><br /></div><div class="gmail_extra">To:<br
/><br/> make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,<br />               LOCKMODE lockmode)<br
/><br/></div><div class="gmail_extra"> That way we can create the new heap with the appropriate relpersistence, so in
theswap_relation_files also copy the persistency, of course.<br /></div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Andafter copy the heap data to the new table (ATRewriteTable) change relpersistence of the
OldHeap'sindexes, because in the "finish_heap_swap" they'll be rebuild.<br /></div><div class="gmail_extra"><br
/></div><divclass="gmail_extra">Thoughts?<br /></div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Regards,<br/><br /></div><div class="gmail_extra">--<br />Fabrízio de Royes Mello<br />
Consultoria/CoachingPostgreSQL<br />>> Timbira: <a href="http://www.timbira.com.br"
target="_blank">http://www.timbira.com.br</a><br/>>> Blog sobre TI: <a href="http://fabriziomello.blogspot.com"
target="_blank">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello"target="_blank">http://br.linkedin.com/in/fabriziomello</a><br />>>
Twitter:<a href="http://twitter.com/fabriziomello" target="_blank">http://twitter.com/fabriziomello</a></div></div> 

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

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

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

<div dir="ltr"><div class="gmail_extra"><br />On Mon, Jul 28, 2014 at 1:41 PM, Christoph Berg <<a
href="mailto:cb@df7cb.de">cb@df7cb.de</a>>wrote:<br />><br />> Re: Fabrízio de Royes Mello 2014-07-28
<CAFcNs+pctx4Q2UYsLOvVFWaznO3U0XhPpkMx5DRhR=<a
href="mailto:Jw8w3tYg@mail.gmail.com">Jw8w3tYg@mail.gmail.com</a>><br/> > > There are something that should I
doon this patch yet?<br />><br />> I haven't got around to have a look at the newest incarnation yet, but<br
/>>I plan to do that soonish. (Of course that shouldn't stop others from<br /> > doing that as well if they
wish.)<br/>><br /><br />Thanks!<br /><br />--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br
/>>>Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br /> >> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/>>> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/> >> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

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/

On 08/21/2014 05:04 PM, Thom Brown wrote:
> On 21 August 2014 14:45, Fabrízio de Royes Mello <fabriziomello@gmail.com>
> wrote:
>
>> 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.
>>
>> 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
>>
>
> To me, that's even more confusing:
>
> CREATE TEMP TABLE test();
> CREATE UNLOGGED TABLE test2();
>
> # ALTER TABLE test SET LOGGED;
> ERROR:  table test is not unlogged
>
> # ALTER TABLE test SET UNLOGGED;
> ERROR:  table test is not permanent
>
> # ALTER TABLE test2 SET UNLOGGED;
> ERROR:  table test2 is not permanent
>
> They're being rejected for different reasons but the error message is
> identical.  Permanent suggests the opposite of temporary, and unlogged
> tables aren't temporary.

In Postgres internals slang, non-permanent means temporary or unlogged. 
But I agree we shouldn't expose users to that term; we use it in the 
docs, and it's not used in command names either.

I wonder if throwing an error is correct behavior anyway. Other ALTER 
TABLE commands just silently do nothing in similar situations, e.g:

lowerdb=# CREATE TABLE foo () WITH OIDS;
CREATE TABLE
lowerdb=# ALTER TABLE foo SET WITH OIDS;
ALTER TABLE

But if we want to throw an error anyway, I'd suggest phrasing it "table 
foo is already unlogged"

- Heikki

Heikki Linnakangas wrote:

> In Postgres internals slang, non-permanent means temporary or
> unlogged. But I agree we shouldn't expose users to that term; we use
> it in the docs, and it's not used in command names either.

Agreed.  I am going over this patch, and the last bit I need to sort out
is the wording of these messages.  I have temporarily settled on this:
ereport(ERROR,        (errcode(ERRCODE_INVALID_TABLE_DEFINITION),         errmsg("cannot change logged status of table
%sto logged",                RelationGetRelationName(rel)),         errdetail("Table %s references unlogged table %s.",
                 RelationGetRelationName(rel),                   RelationGetRelationName(relfk))));
 

Note the term "logged status" to talk about whether a table is logged or
not.  I thought about "loggedness" but I'm not sure english speakers are
going to love me for that.  Any other ideas there?

> I wonder if throwing an error is correct behavior anyway. Other
> ALTER TABLE commands just silently do nothing in similar situations,
> e.g:
> 
> lowerdb=# CREATE TABLE foo () WITH OIDS;
> CREATE TABLE
> lowerdb=# ALTER TABLE foo SET WITH OIDS;
> ALTER TABLE
> 
> But if we want to throw an error anyway, I'd suggest phrasing it
> "table foo is already unlogged"

Yeah, there is precedent for silently doing nothing.  We previously
threw warnings or notices, but nowadays even that is gone.  Throwing an
error definitely seems the wrong thing.  In the patch I have it's like
this:
ereport(ERROR,        (errcode(ERRCODE_INVALID_TABLE_DEFINITION),         errmsg("cannot change logged status of table
%sto unlogged",                RelationGetRelationName(rel)),         errdetail("Table %s is already unlogged.",
          RelationGetRelationName(rel))));
 

but I think I will just take that out as a whole and set a flag to
indicate nothing is to be done.

(This also means that setting tab->rewrite while analyzing the command
is the wrong thing to do.  Instead, the test for tab->rewrite should
have an || tab->chgLoggedness test, and we set chgLoggedness off if we
see that it's a no-op.  That way we avoid a pointless table rewrite and
a pointless error in a multi-command ALTER TABLE that has a no-op SET
LOGGED subcommand among other things.)


I changed the doc item in ALTER TABLE,
  <varlistentry>   <term><literal>SET {LOGGED | UNLOGGED}</literal></term>   <listitem>    <para>     This form changes
thetable from unlogged to logged or vice-versa     (see <xref linkend="SQL-CREATETABLE-UNLOGGED">).  It cannot be
applied    to a temporary table.    </para>   </listitem>  </varlistentry>
 

I removed the fact that it needs ACCESS EXCLUSIVE because that's already
mentioned in the introductory paragraph.  I also removed the phrase that
it requires a table rewrite, because on reading existing text I noticed
that we don't document which forms cause rewrites.  Perhaps we should
document that somehow, but adding it to only one item seems wrong.

I will post an updated patch as soon as I fix a bug I introduced in the
check for FKs.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Agreed.  I am going over this patch, and the last bit I need to sort out
> is the wording of these messages.  I have temporarily settled on this:

>     ereport(ERROR,
>             (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
>              errmsg("cannot change logged status of table %s to logged",
>                     RelationGetRelationName(rel)),
>              errdetail("Table %s references unlogged table %s.",
>                        RelationGetRelationName(rel),
>                        RelationGetRelationName(relfk))));

> Note the term "logged status" to talk about whether a table is logged or
> not.  I thought about "loggedness" but I'm not sure english speakers are
> going to love me for that.  Any other ideas there?

Just say "cannot change status of table %s to logged".

> Yeah, there is precedent for silently doing nothing.  We previously
> threw warnings or notices, but nowadays even that is gone.  Throwing an
> error definitely seems the wrong thing.

Agreed, just do nothing if it's already the right setting.
        regards, tom lane

On 2014-08-21 16:59:17 -0400, Alvaro Herrera wrote:
> Heikki Linnakangas wrote:
> 
> > In Postgres internals slang, non-permanent means temporary or
> > unlogged. But I agree we shouldn't expose users to that term; we use
> > it in the docs, and it's not used in command names either.
> 
> Agreed.  I am going over this patch, and the last bit I need to sort out
> is the wording of these messages.  I have temporarily settled on this:
> 
>     ereport(ERROR,
>             (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
>              errmsg("cannot change logged status of table %s to logged",
>                     RelationGetRelationName(rel)),
>              errdetail("Table %s references unlogged table %s.",
>                        RelationGetRelationName(rel),
>                        RelationGetRelationName(relfk))));

Maybe 'cannot change persistency of table .. from unlogged to logged'; possibly with
s/persistency/durability/?


Have you looked at the correctness of the patch itself? Last time I'd
looked it has fundamental correctness issues. I'd outlined a possible
solution, but I haven't looked since.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Agreed.  I am going over this patch, and the last bit I need to sort out
> > is the wording of these messages.  I have temporarily settled on this:
>
> >     ereport(ERROR,
> >             (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> >              errmsg("cannot change logged status of table %s to logged",
> >                     RelationGetRelationName(rel)),
> >              errdetail("Table %s references unlogged table %s.",
> >                        RelationGetRelationName(rel),
> >                        RelationGetRelationName(relfk))));
>
> > Note the term "logged status" to talk about whether a table is logged or
> > not.  I thought about "loggedness" but I'm not sure english speakers are
> > going to love me for that.  Any other ideas there?
>
> Just say "cannot change status of table %s to logged".

I like this one, thanks.

> > Yeah, there is precedent for silently doing nothing.  We previously
> > threw warnings or notices, but nowadays even that is gone.  Throwing an
> > error definitely seems the wrong thing.
>
> Agreed, just do nothing if it's already the right setting.

Done that way.

Andres Freund wrote:

> Have you looked at the correctness of the patch itself? Last time I'd
> looked it has fundamental correctness issues. I'd outlined a possible
> solution, but I haven't looked since.

Yeah, Fabrizio had it passing the relpersistence down to make_new_heap,
so the transient table is created with the right setting.  AFAICS it's
good now.  I'm a bit uneasy about the way it changes indexes: it just
updates pg_class for them just before invoking the reindex in
finish_heap_swap.  I think it's correct as it stands though; at least,
the rewrite phase happens with the right setting, so that if there are
constraints being checked and these invoke index scans, such accesses
would not leave buffers with the wrong setting in shared_buffers.

Another option would be to pass the new relpersistence down to
finish_heap_swap, but that would be hugely complicated AFAICS.

Here's the updated patch.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

<div dir="ltr"><br />On Thu, Aug 21, 2014 at 8:04 PM, Alvaro Herrera <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>wrote:<br />> Andres Freund wrote:<br
/>><br/>> > Have you looked at the correctness of the patch itself? Last time I'd<br /> > > looked it
hasfundamental correctness issues. I'd outlined a possible<br />> > solution, but I haven't looked since.<br
/>><br/>> Yeah, Fabrizio had it passing the relpersistence down to make_new_heap,<br /> > so the transient
tableis created with the right setting.  AFAICS it's<br />> good now.  I'm a bit uneasy about the way it changes
indexes:it just<br />> updates pg_class for them just before invoking the reindex in<br /> > finish_heap_swap.  I
thinkit's correct as it stands though; at least,<br />> the rewrite phase happens with the right setting, so that if
thereare<br />> constraints being checked and these invoke index scans, such accesses<br /> > would not leave
bufferswith the wrong setting in shared_buffers.<br />><br /><br />Ok.<br /><br />> Another option would be to
passthe new relpersistence down to<br />> finish_heap_swap, but that would be hugely complicated AFAICS.<br />
><br/><br />I think isn't so complicated to do it, but will this improve something ? <br />Maybe I didn't understand
itvery well. IMHO it just complicate a<br />simple thing.<br /><br /><br />> Here's the updated patch.<br /> ><br
/><br/>Thanks Alvaro!<br /><br />Regards,<br /><br />--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching
PostgreSQL<br/>>> Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br />>> Blog:
<ahref="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br /> >> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div>

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?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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?

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.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:

> Hmm.  I confess to not having paid enough attention to this,

Sorry about that.  I guess I should somehow flag threads "I'm planning
to commit this" so that other people can review stuff carefully.

> but:
>
> 1. Loggedness is not a word.  I think that "persistence" or
> "relpersistence" would be better.

Yeah, AFAICS this is only used in the variable chgLoggedness and a
couple of functions.  I don't think we're tense about unwordness of
words we use in source code (as opposed to error messages and docs), and
we have lots of russianisms left by Vadim and others, so I didn't see it
as a serious issue.  But then I'm not a native english speaker and I'm
not bothered by it.  OTOH I came up with "loggedness" on my own -- you
wouldn't have seen it even in Fabrizio's latest version of the patch.

Who knows, it might become a real english word one day.

You want me to change that to chgPersistence and so on?  No prob, just
LMK.

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

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Fabrízio de Royes Mello wrote:
> On Fri, Aug 22, 2014 at 4:22 PM, Robert Haas <robertmhaas@gmail.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.
> 
> Because of this concern I implement a solution pointed by Andres [1].

Actually what you did was better than what he suggested.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

<div dir="ltr"><div class="gmail_extra"><br />On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>wrote:<br />><br />> > 2. The patch
seemsto think that it can sometimes be safe to change<br /> > > the relpersistence of an existing relation.
 Unlessyou can be sure<br />> > that no buffers can possibly be present in shared_buffers and nobody<br />>
>will use an existing relcache entry to read a new one in, it's not,<br /> > > because the buffers won't have
theright BM_PERSISTENT marking.  I'm<br />> > very nervous about the fact that this patch seems not to have
touched<br/>> > bufmgr.c, but maybe I'm missing something.<br /> ><br />> Right.  Andres pointed this out
previously,and the patch was updated<br />> consequently.  The only remaining case in which we do that, again
AFAIR,<br/>> is for indexes, just after the table has been rewritten and just before<br /> > the indexes are
reindexed. There should be no buffer access of the old<br />> indexes at that point, so there would be no buffer
markedwith the wrong<br />> flag.  Also, the table is locked access-exclusively (is that a word?),<br /> > so no
othertransaction could possibly be reading buffers for its<br />> indexes.<br />><br />> I pointed out, in the
emailjust before pushing the patch, that perhaps<br />> we should pass down the new relpersistence flag into
finish_heap_swap,<br/> > and from there we could pass it down to reindex_index() which is where<br />> it would
beneeded.  I'm not sure it's worth the trouble, but I think we<br />> can still ask Fabrizio to rework that part.<br
/>><br/> > Maybe it is me missing something.<br />><br />> BTW why is it that index_build() checks the
heap'srelpersistence flag<br />> rather than the index'?<br />><br /><br /></div><div class="gmail_extra">I can
reworkthis part if it's a real concern.<br /></div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Regards,<br/></div><div class="gmail_extra"><br />--<br />Fabrízio de Royes Mello<br
/>Consultoria/CoachingPostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/> >> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/> >> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas <robertmhaas@gmail.com> writes:
> 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.

Maybe I misunderstood something, but I had the impression that this was
handled by assigning a new relfilenode (and hence copying all the data).
So the buffers with one marking would be disjoint from the ones with the
other marking.
        regards, tom lane

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Robert Haas wrote:
>> 1. Loggedness is not a word.  I think that "persistence" or
>> "relpersistence" would be better.

> You want me to change that to chgPersistence and so on?  No prob, just
> LMK.

+1 for s/loggedness/persistence/ -- I agree with Robert that it's
a bit grating on the native ear.
        regards, tom lane

<div dir="ltr"><div class="gmail_extra"><br />On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>wrote:<br />><br />> BTW why is it that
index_build()checks the heap's relpersistence flag<br /> > rather than the index'?<br />><br /><br /></div><div
class="gmail_extra">I'mcurious about it too... the code in src/backend/catalog/index.c is:<br /><br />1975     if
(heapRelation->rd_rel->relpersistence== RELPERSISTENCE_UNLOGGED &&<br /> 1976        
!smgrexists(indexRelation->rd_smgr,INIT_FORKNUM))<br />1977     {<br /><br /></div><div class="gmail_extra"><br
/>Shouldnot to be in that way?<br /><br />1975     if (indexRelation->rd_rel->relpersistence ==
RELPERSISTENCE_UNLOGGED&&<br /> 1976         !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM))<br
/>1977    {<br /><br /></div><div class="gmail_extra"><br />Alvaro, is this your concern? Right?<br /></div><div
class="gmail_extra"><br/></div><div class="gmail_extra"> Regards,<br /><br /></div><div class="gmail_extra">--<br
/>Fabríziode Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/> >> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>

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

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.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

<div dir="ltr"><div class="gmail_extra"><br />On Mon, Aug 25, 2014 at 2:55 PM, Alvaro Herrera <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>wrote:<br />><br />> Fabrízio de Royes
Mellowrote:<br /> > > On Sat, Aug 23, 2014 at 8:53 AM, Michael Paquier <<a
href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>><br/>> > wrote:<br />> > ><br
/>>> > On Sat, Aug 23, 2014 at 3:32 AM, Alvaro Herrera<br /> > > > <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>wrote:<br />> > > > Great.  Pushed.
 Thanksfor the patch.<br />> > > There is a typo in what has been pushed. Patch attached.<br /> > >
><br/>> ><br />> > Thanks... I fixed that in my last patch to change 'loggedness' to<br />> >
'persistence'.Attached.<br />><br />> Thanks, pushed.  I also added a comment explaining why it's okay to do<br
/>> what we're doing.<br />><br /><br /></div><div class="gmail_extra">Thanks...<br /><br /></div><div
class="gmail_extra">I'mworking on a refactoring to pass down the relpersistence flag to finish_heap_swap... is this
validfor now or I should leave it to another patch?<br /></div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Regards,<br/><br /></div><div class="gmail_extra">--<br />Fabrízio de Royes Mello<br
/>Consultoria/CoachingPostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/> >> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/> >> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>

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.
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.
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;
Regards,
--
Michael

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!

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Alvaro Herrera 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!

The buildfarm says -DCLOBBER_CACHE_ALWAYS does not like this patch.

-- 
Christian

On Mon, Nov 17, 2014 at 5:56 AM, Christian Ullrich <chris@chrullrich.net> wrote:
> * Alvaro Herrera 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!
>
>
> The buildfarm says -DCLOBBER_CACHE_ALWAYS does not like this patch.
The complaint comes from jaguarundi, like here:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi&dt=2014-11-16%2015%3A33%3A23

Adding a new parameter to RelationSetNewRelFile
As Tom mentioned, adding a new parameter to set the persistence
through RelationSetNewRelfilenode works. Patch, actually tested with
CLOBBER_CACHE_ALWAYS, attached.
http://www.postgresql.org/message-id/27238.1416073917@sss.pgh.pa.us
Regards,
--
Michael

Michael Paquier wrote:

> The complaint comes from jaguarundi, like here:
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi&dt=2014-11-16%2015%3A33%3A23
> 
> As Tom mentioned, adding a new parameter to set the persistence
> through RelationSetNewRelfilenode works. Patch, actually tested with
> CLOBBER_CACHE_ALWAYS, attached.

I wrote an identical patch on Saturday and watched it pass
CLOBBER_CACHE_ALWAYS test on Sunday.  But the reason I didn't push is I
couldn't understand *why* is there a problem here.  I imagine that the
source of the issue is supposed to be a relcache invalidation that takes
place (in the original code) after reindex_index changes relpersistence,
and before RelationSetNewRelfilenode() creates the filenode.  But at
what point does that code absorb invalidation messages?  Or is there a
completely different mechanism that causes the problem?  If so, what?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera wrote:

> I wrote an identical patch on Saturday and watched it pass
> CLOBBER_CACHE_ALWAYS test on Sunday.  But the reason I didn't push is I
> couldn't understand *why* is there a problem here.  I imagine that the
> source of the issue is supposed to be a relcache invalidation that takes
> place (in the original code) after reindex_index changes relpersistence,
> and before RelationSetNewRelfilenode() creates the filenode.  But at
> what point does that code absorb invalidation messages?  Or is there a
> completely different mechanism that causes the problem?  If so, what?

Ah, it's the anti-collision stuff in GetNewOid(), isn't it.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services