[GSoC2014] Patch ALTER TABLE ... SET LOGGED
Hi all,
As part of GSoC2014 I'm sending a patch to add the capability of change an
unlogged table to logged [1]https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014.
I'll add it to the 9.5CF1.
Regards,
[1]: https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014
https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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
Attachments:
gsoc2014_alter_table_set_logged_v1.patchtext/x-diff; charset=US-ASCII; name=gsoc2014_alter_table_set_logged_v1.patchDownload+251-4
On Wed, Jun 11, 2014 at 1:19 PM, Fabrízio de Royes Mello <
fabriziomello@gmail.com> wrote:
Hi all,
As part of GSoC2014 I'm sending a patch to add the capability of change an
unlogged table to logged [1].
Hi all,
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.
With this patch we finish the main goals of the GSoC2014 and now we'll work
in the additional goals.
Regards,
[1]: https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014
https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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
Attachments:
gsoc2014_alter_table_set_logged_v2.patchtext/x-diff; charset=US-ASCII; name=gsoc2014_alter_table_set_logged_v2.patchDownload+382-14
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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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]https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014.
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 OIDSThis 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.outPlease 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
https://wiki.postgresql.org/wiki/Allow_an_unlogged_table_to_be_changed_to_logged_GSoC_2014
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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
Attachments:
gsoc2014_alter_table_set_logged_v3.patchtext/x-diff; charset=US-ASCII; name=gsoc2014_alter_table_set_logged_v3.patchDownload+426-14
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.sgmlb/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 ] <replaceableclass="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 OIDSThis 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 indexesThis 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/
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.sgmlb/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 ] <replaceableclass="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 OIDSThis 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.
Fixed.
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
Show quoted text
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
Attachments:
gsoc2014_alter_table_set_logged_v4.patchtext/x-diff; charset=US-ASCII; name=gsoc2014_alter_table_set_logged_v4.patchDownload+360-3
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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Show quoted text
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
Attachments:
gsoc2014_alter_table_set_logged_v5.patchtext/x-diff; charset=US-ASCII; name=gsoc2014_alter_table_set_logged_v5.patchDownload+364-3
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' 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
...
+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' 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
+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
...
+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' ORDER BY 1; +DROP TABLE logged3; +DROP TABLE logged2; +DROP TABLE logged1;
I think we are almost there :)
Christoph
--
cb@df7cb.de | http://www.df7cb.de/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 precondictionspreconditions (without trailing space :)
Fixed.
+ * to perform the operation: + * - check if the target table is unlogged/permanentepermanent
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 permamentafter 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 tablea 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 unloggedafter changing
Fixed.
I think we are almost there :)
Yeah... thanks a lot for your help.
Att,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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
Attachments:
gsoc2014_alter_table_set_logged_v6.patchtext/x-diff; charset=US-ASCII; name=gsoc2014_alter_table_set_logged_v6.patchDownload+373-3
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 precondictionspreconditions (without trailing space :)
Fixed.
+ * to perform the operation: + * - check if the target table is unlogged/permanentepermanent
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 tablea permanent
Fixed.
+SELECT relname, relpersistence, oid = relfilenode AS
original_relfilenode FROM pg_class WHERE relname ~ '^logged1' ORDER BY 1;
+CREATE TABLE logged2(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES
logged1); -- fk reference
+CREATE TABLE logged3(f1 SERIAL PRIMARY KEY, f2 INTEGER REFERENCES
logged3); -- self fk reference
+ALTER TABLE logged1 SET UNLOGGED; -- fails because exists a FK from
a permanent table
...
Fixed.
+ALTER TABLE logged3 SET UNLOGGED; -- skip self FK reference +ALTER TABLE logged2 SET UNLOGGED; +ALTER TABLE logged1 SET UNLOGGED; +-- check relpersistence of a permanent table after changed to
unlogged
after changing
Fixed.
I think we are almost there :)
Yeah... thanks a lot for your help.
The previous patch (v6) has some trailing spaces... fixed.
(sorry by the noise)
Att,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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
Attachments:
gsoc2014_alter_table_set_logged_v7.patchtext/x-diff; charset=US-ASCII; name=gsoc2014_alter_table_set_logged_v7.patchDownload+373-3
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.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Show quoted text
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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?
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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 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 fromcache,
not even when they're dirty.
b) When converting from a unlogged to a logged table the relationneeds
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 WALlogged.
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 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.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 <replaceableclass="PARAMETER">parent_table</replaceable>
OF <replaceable class="PARAMETER">type_name</replaceable>
NOT OF
OWNER TO <replaceable class="PARAMETER">new_owner</replaceable>
- SET TABLESPACE <replaceableclass="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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Show quoted text
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
