Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Mon, Nov 17, 2014 at 5:56 AM, Christian Ullrich ch...@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=jaguarundidt=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.1416073...@sss.pgh.pa.us Regards, -- Michael From a1a7e4182bd1b5d66260220f040aee4a35e91c75 Mon Sep 17 00:00:00 2001 From: Michael Paquier mpaqu...@otacoo.com Date: Mon, 17 Nov 2014 17:04:05 + Subject: [PATCH] Fix CLOBBER_CACHE_ALWAYS broken by 85b506b Previous commit that removed ATChangeIndexesPersistence to replace it with a lower-level API able to define a new relpersistence for a relation when reindexing it failed with CLOBBER_CACHE_ALWAYS enabled. This patch fixes it by setting the relpersistence of the newly-created relation after its relfilenode is created by passing a new parameter to RelationSetNewRelfilenode able to set the persistence of a relation. --- src/backend/catalog/index.c| 5 + src/backend/commands/sequence.c| 3 ++- src/backend/commands/tablecmds.c | 6 -- src/backend/utils/cache/relcache.c | 6 +++--- src/include/utils/relcache.h | 4 +++- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index b57aa95..825235a 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -3191,12 +3191,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence) indexInfo-ii_ExclusionStrats = NULL; } - /* Set the relpersistence of the new index */ - iRel-rd_rel-relpersistence = persistence; - /* We'll build a new physical relation for the index */ RelationSetNewRelfilenode(iRel, InvalidTransactionId, - InvalidMultiXactId); + InvalidMultiXactId, persistence); /* Initialize the index and rebuild */ /* Note: we do not need to re-establish pkey setting */ diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c index e5f7765..4f7f586 100644 --- a/src/backend/commands/sequence.c +++ b/src/backend/commands/sequence.c @@ -304,7 +304,8 @@ ResetSequence(Oid seq_relid) * Same with relminmxid, since a sequence will never contain multixacts. */ RelationSetNewRelfilenode(seq_rel, InvalidTransactionId, - InvalidMultiXactId); + InvalidMultiXactId, + seq_rel-rd_rel-relpersistence); /* * Insert the modified tuple into the new storage file. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 093224f..c57751e 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1196,7 +1196,8 @@ ExecuteTruncate(TruncateStmt *stmt) * as the relfilenode value. The old storage file is scheduled for * deletion at commit. */ - RelationSetNewRelfilenode(rel, RecentXmin, minmulti); + RelationSetNewRelfilenode(rel, RecentXmin, minmulti, + rel-rd_rel-relpersistence); if (rel-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED) heap_create_init_fork(rel); @@ -1209,7 +1210,8 @@ ExecuteTruncate(TruncateStmt *stmt) if (OidIsValid(toast_relid)) { rel = relation_open(toast_relid, AccessExclusiveLock); -RelationSetNewRelfilenode(rel, RecentXmin, minmulti); +RelationSetNewRelfilenode(rel, RecentXmin, minmulti, + rel-rd_rel-relpersistence); if (rel-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED) heap_create_init_fork(rel); heap_close(rel, NoLock); diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 2250c56..fa75771 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -3008,7 +3008,7 @@ RelationBuildLocalRelation(const char *relname, */ void RelationSetNewRelfilenode(Relation relation, TransactionId freezeXid, - MultiXactId minmulti) + MultiXactId minmulti, char relpersistence) { Oid newrelfilenode;
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Michael Paquier wrote: The complaint comes from jaguarundi, like here: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundidt=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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
* 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Sat, Nov 15, 2014 at 2:23 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Michael Paquier wrote: Btw, perhaps this diff should be pushed as a different patch as this is a rather different thing: - if (heapRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED + if (indexRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED !smgrexists(indexRelation-rd_smgr, INIT_FORKNUM) As this is a correctness fix, it does not seem necessary to back-patch it: the parent relation always has the same persistence as its indexes. There was an argument for doing it this way that only applies if this patch went in, but I can't remember now what it was. Anyway I pushed the patch after some slight additional changes. Thanks! Thanks! -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Thu, Nov 6, 2014 at 3:42 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Sep 13, 2014 at 11:02 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: Patch rebased and added to commitfest [1]. It looks like a good thing to remove ATChangeIndexesPersistence, this puts the persistence switch directly into reindex process. A couple of minor comments about this patch: 1) Reading it, I am wondering if it would not be finally time to switch to a macro to get a relation's persistence, something like RelationGetPersistence in rel.h... Not related directly to this patch. Good idea... I'll provide a patch soon. 2) reindex_index has as new argument a relpersislence value for the new index. reindex_relation has differently a new set of flags to enforce the relpersistence of all the underling indexes. Wouldn't it be better for API consistency to pass directly a relpersistence value through reindex_relation? In any case, the comment block of reindex_relation does not contain a description of the new flags. I did it because the ReindexDatabase build a list of oids to run reindex_relation for each item of the list. I can change the list to store the relpersistence also, but this can lead us to a concurrency trouble because if one session execute REINDEX DATABASE and other session run ALTER TABLE name SET {LOGGED|UNLOGGED} during the building of the list the reindex can lead us to a inconsitence state. Added comments to comment block of reindex_relation. 3) Here you may as well just set the value and be done: +/* + * Check if need to set the new relpersistence + */ +if (iRel-rd_rel-relpersistence != relpersistence) +iRel-rd_rel-relpersistence = relpersistence; Hmmm... I really don't know why I did it... fixed. Thanks! -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 912038a..50cf0ef 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1980,7 +1980,7 @@ index_build(Relation heapRelation, * created it, or truncated twice in a subsequent transaction, the * relfilenode won't change, and nothing needs to be done here. */ - if (heapRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED + if (indexRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED !smgrexists(indexRelation-rd_smgr, INIT_FORKNUM)) { RegProcedure ambuildempty = indexRelation-rd_am-ambuildempty; @@ -3130,7 +3130,7 @@ IndexGetRelation(Oid indexId, bool missing_ok) * reindex_index - This routine is used to recreate a single index */ void -reindex_index(Oid indexId, bool skip_constraint_checks) +reindex_index(Oid indexId, bool skip_constraint_checks, char relpersistence) { Relation iRel, heapRelation; @@ -3191,6 +3191,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks) indexInfo-ii_ExclusionStrats = NULL; } + /* Set the relpersistence of the new index */ + iRel-rd_rel-relpersistence = relpersistence; + /* We'll build a new physical relation for the index */ RelationSetNewRelfilenode(iRel, InvalidTransactionId, InvalidMultiXactId); @@ -3310,6 +3313,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks) * performance, other callers should include the flag only after transforming * the data in a manner that risks a change in constraint validity. * + * REINDEX_REL_FORCE_UNLOGGED: if true, force the new rebuilded indexes to be + * unlogged. + * + * REINDEX_REL_FORCE_LOGGED: if true, force the new rebuilded indexes to be + * permanent. + * * Returns true if any indexes were rebuilt (including toast table's index * when relevant). Note that a CommandCounterIncrement will occur after each * index rebuild. @@ -3389,11 +3398,19 @@ reindex_relation(Oid relid, int flags) foreach(indexId, indexIds) { Oid indexOid = lfirst_oid(indexId); + char relpersistence = rel-rd_rel-relpersistence; if (is_pg_class) RelationSetIndexList(rel, doneIndexes, InvalidOid); - reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS)); + /* Check for flags to force UNLOGGED or PERMANENT persistence */ + if (flags REINDEX_REL_FORCE_UNLOGGED) +relpersistence = RELPERSISTENCE_UNLOGGED; + else if (flags REINDEX_REL_FORCE_PERMANENT) +relpersistence = RELPERSISTENCE_PERMANENT; + + reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS), + relpersistence); CommandCounterIncrement(); diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 6a578ec..95b9a4f 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -589,7 +589,8 @@ rebuild_relation(Relation OldHeap, Oid
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Thanks for the updated patch, Fabrizio. On Tue, Nov 11, 2014 at 7:44 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: A couple of minor comments about this patch: 1) Reading it, I am wondering if it would not be finally time to switch to a macro to get a relation's persistence, something like RelationGetPersistence in rel.h... Not related directly to this patch. Good idea... I'll provide a patch soon. I created a thread dedicated to that: http://www.postgresql.org/message-id/cab7npqseb+hwitxfwkqypa7+9bjolnxio47qsro3hcbsoq0...@mail.gmail.com Now few people cared enough to comment :) 2) reindex_index has as new argument a relpersislence value for the new index. reindex_relation has differently a new set of flags to enforce the relpersistence of all the underling indexes. Wouldn't it be better for API consistency to pass directly a relpersistence value through reindex_relation? In any case, the comment block of reindex_relation does not contain a description of the new flags. I did it because the ReindexDatabase build a list of oids to run reindex_relation for each item of the list. I can change the list to store the relpersistence also, but this can lead us to a concurrency trouble because if one session execute REINDEX DATABASE and other session run ALTER TABLE name SET {LOGGED|UNLOGGED} during the building of the list the reindex can lead us to a inconsistent state. Fair point. I forgot this code path. Except a typo with s/rebuilded/rebuilt/ in the patch, corrected in the patch attached with some extra comments bundled, it seems to be that this patch can be passed to a committer, so marking it so. 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. Regards, -- Michael diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 912038a..a0a81e8 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1980,7 +1980,7 @@ index_build(Relation heapRelation, * created it, or truncated twice in a subsequent transaction, the * relfilenode won't change, and nothing needs to be done here. */ - if (heapRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED + if (indexRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED !smgrexists(indexRelation-rd_smgr, INIT_FORKNUM)) { RegProcedure ambuildempty = indexRelation-rd_am-ambuildempty; @@ -3130,7 +3130,7 @@ IndexGetRelation(Oid indexId, bool missing_ok) * reindex_index - This routine is used to recreate a single index */ void -reindex_index(Oid indexId, bool skip_constraint_checks) +reindex_index(Oid indexId, bool skip_constraint_checks, char relpersistence) { Relation iRel, heapRelation; @@ -3191,6 +3191,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks) indexInfo-ii_ExclusionStrats = NULL; } + /* Set the relpersistence of the new index */ + iRel-rd_rel-relpersistence = relpersistence; + /* We'll build a new physical relation for the index */ RelationSetNewRelfilenode(iRel, InvalidTransactionId, InvalidMultiXactId); @@ -3310,6 +3313,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks) * performance, other callers should include the flag only after transforming * the data in a manner that risks a change in constraint validity. * + * REINDEX_REL_FORCE_UNLOGGED: if true, set the persistence of the rebuilt + * indexes to unlogged. + * + * REINDEX_REL_FORCE_LOGGED: if true, set the persistence of the rebuilt + * indexes to permanent. + * * Returns true if any indexes were rebuilt (including toast table's index * when relevant). Note that a CommandCounterIncrement will occur after each * index rebuild. @@ -3389,11 +3398,19 @@ reindex_relation(Oid relid, int flags) foreach(indexId, indexIds) { Oid indexOid = lfirst_oid(indexId); + char relpersistence = rel-rd_rel-relpersistence; if (is_pg_class) RelationSetIndexList(rel, doneIndexes, InvalidOid); - reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS)); + /* Check for flags to enforce UNLOGGED or PERMANENT persistence */ + if (flags REINDEX_REL_FORCE_UNLOGGED) +relpersistence = RELPERSISTENCE_UNLOGGED; + else if (flags REINDEX_REL_FORCE_PERMANENT) +relpersistence = RELPERSISTENCE_PERMANENT; + + reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS), + relpersistence); CommandCounterIncrement(); diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 6a578ec..e067abc 100644 --- a/src/backend/commands/cluster.c +++
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Sat, Sep 13, 2014 at 11:02 PM, Fabrízio de Royes Mello fabriziome...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Tue, Aug 26, 2014 at 1:42 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Fri, Aug 22, 2014 at 5:23 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fabrízio de Royes Mello wrote: On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I pointed out, in the email just before pushing the patch, that perhaps we should pass down the new relpersistence flag into finish_heap_swap, and from there we could pass it down to reindex_index() which is where it would be needed. I'm not sure it's worth the trouble, but I think we can still ask Fabrizio to rework that part. I can rework this part if it's a real concern. I guess we can make a better assessment by seeing what it would take. I'm afraid it will turn out to be really ugly. Now, there's some desire to have unlogged indexes on logged tables; I guess if we have that, then eventually there will also be a desire to swap individual indexes from logged to unlogged. Perhaps whatever fix we come up with here would pave the road for that future feature. Álvaro, I did a refactoring to pass down the relpersistence to finish_heap_swap and then change the catalog inside the reindex_index instead of in the rewrite table phase. That way we can get rid the function ATChangeIndexesPersistence in the src/backend/commands/tablecmds.c. Thoughts? Patch rebased and added to commitfest [1]. Regards, [1] https://commitfest.postgresql.org/action/commitfest_view?id=24 -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index ee10594..173f412 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1973,7 +1973,7 @@ index_build(Relation heapRelation, * created it, or truncated twice in a subsequent transaction, the * relfilenode won't change, and nothing needs to be done here. */ - if (heapRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED + if (indexRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED !smgrexists(indexRelation-rd_smgr, INIT_FORKNUM)) { RegProcedure ambuildempty = indexRelation-rd_am-ambuildempty; @@ -3099,7 +3099,7 @@ IndexGetRelation(Oid indexId, bool missing_ok) * reindex_index - This routine is used to recreate a single index */ void -reindex_index(Oid indexId, bool skip_constraint_checks) +reindex_index(Oid indexId, bool skip_constraint_checks, char relpersistence) { Relation iRel, heapRelation; @@ -3160,6 +3160,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks) indexInfo-ii_ExclusionStrats = NULL; } + /* + * Check if need to set the new relpersistence + */ + if (iRel-rd_rel-relpersistence != relpersistence) + iRel-rd_rel-relpersistence = relpersistence; + /* We'll build a new physical relation for the index */ RelationSetNewRelfilenode(iRel, InvalidTransactionId, InvalidMultiXactId); @@ -3358,11 +3364,19 @@ reindex_relation(Oid relid, int flags) foreach(indexId, indexIds) { Oid indexOid = lfirst_oid(indexId); + char relpersistence = rel-rd_rel-relpersistence; if (is_pg_class) RelationSetIndexList(rel, doneIndexes, InvalidOid); - reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS)); + /* Check for flags to force UNLOGGED or PERMANENT persistence */ + if (flags REINDEX_REL_FORCE_UNLOGGED) +relpersistence = RELPERSISTENCE_UNLOGGED; + else if (flags REINDEX_REL_FORCE_PERMANENT) +relpersistence = RELPERSISTENCE_PERMANENT; + + reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS), + relpersistence); CommandCounterIncrement(); diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index ff80b09..f285026 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -588,7 +588,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) */ finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog, swap_toast_by_content, false, true, - frozenXid, cutoffMulti); + frozenXid, cutoffMulti, + OldHeap-rd_rel-relpersistence); } @@ -1474,7 +1475,8 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, bool check_constraints, bool is_internal, TransactionId frozenXid, - MultiXactId cutoffMulti) + MultiXactId cutoffMulti, + char newrelpersistence) { ObjectAddress object; Oid mapped_tables[4]; @@ -1518,6 +1520,12 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, reindex_flags = REINDEX_REL_SUPPRESS_INDEX_USE; if (check_constraints) reindex_flags |= REINDEX_REL_CHECK_CONSTRAINTS; + + if (newrelpersistence == RELPERSISTENCE_UNLOGGED) + reindex_flags |=
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Fabrízio de Royes Mello wrote: On Sat, Aug 23, 2014 at 8:53 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Aug 23, 2014 at 3:32 AM, Alvaro Herrera alvhe...@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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Mon, Aug 25, 2014 at 2:55 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fabrízio de Royes Mello wrote: On Sat, Aug 23, 2014 at 8:53 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Aug 23, 2014 at 3:32 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Great. Pushed. Thanks for the patch. There is a typo in what has been pushed. Patch attached. Thanks... I fixed that in my last patch to change 'loggedness' to 'persistence'. Attached. Thanks, pushed. I also added a comment explaining why it's okay to do what we're doing. Thanks... I'm working on a refactoring to pass down the relpersistence flag to finish_heap_swap... is this valid for now or I should leave it to another patch? Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Fri, Aug 22, 2014 at 5:23 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fabrízio de Royes Mello wrote: On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I pointed out, in the email just before pushing the patch, that perhaps we should pass down the new relpersistence flag into finish_heap_swap, and from there we could pass it down to reindex_index() which is where it would be needed. I'm not sure it's worth the trouble, but I think we can still ask Fabrizio to rework that part. I can rework this part if it's a real concern. I guess we can make a better assessment by seeing what it would take. I'm afraid it will turn out to be really ugly. Now, there's some desire to have unlogged indexes on logged tables; I guess if we have that, then eventually there will also be a desire to swap individual indexes from logged to unlogged. Perhaps whatever fix we come up with here would pave the road for that future feature. Álvaro, I did a refactoring to pass down the relpersistence to finish_heap_swap and then change the catalog inside the reindex_index instead of in the rewrite table phase. That way we can get rid the function ATChangeIndexesPersistence in the src/backend/commands/tablecmds.c. Thoughts? -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index ee10594..173f412 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1973,7 +1973,7 @@ index_build(Relation heapRelation, * created it, or truncated twice in a subsequent transaction, the * relfilenode won't change, and nothing needs to be done here. */ - if (heapRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED + if (indexRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED !smgrexists(indexRelation-rd_smgr, INIT_FORKNUM)) { RegProcedure ambuildempty = indexRelation-rd_am-ambuildempty; @@ -3099,7 +3099,7 @@ IndexGetRelation(Oid indexId, bool missing_ok) * reindex_index - This routine is used to recreate a single index */ void -reindex_index(Oid indexId, bool skip_constraint_checks) +reindex_index(Oid indexId, bool skip_constraint_checks, char relpersistence) { Relation iRel, heapRelation; @@ -3160,6 +3160,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks) indexInfo-ii_ExclusionStrats = NULL; } + /* + * Check if need to set the new relpersistence + */ + if (iRel-rd_rel-relpersistence != relpersistence) + iRel-rd_rel-relpersistence = relpersistence; + /* We'll build a new physical relation for the index */ RelationSetNewRelfilenode(iRel, InvalidTransactionId, InvalidMultiXactId); @@ -3358,11 +3364,19 @@ reindex_relation(Oid relid, int flags) foreach(indexId, indexIds) { Oid indexOid = lfirst_oid(indexId); + char relpersistence = rel-rd_rel-relpersistence; if (is_pg_class) RelationSetIndexList(rel, doneIndexes, InvalidOid); - reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS)); + /* Check for flags to force UNLOGGED or PERMANENT persistence */ + if (flags REINDEX_REL_FORCE_UNLOGGED) +relpersistence = RELPERSISTENCE_UNLOGGED; + else if (flags REINDEX_REL_FORCE_PERMANENT) +relpersistence = RELPERSISTENCE_PERMANENT; + + reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS), + relpersistence); CommandCounterIncrement(); diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index ff80b09..f285026 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -588,7 +588,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) */ finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog, swap_toast_by_content, false, true, - frozenXid, cutoffMulti); + frozenXid, cutoffMulti, + OldHeap-rd_rel-relpersistence); } @@ -1474,7 +1475,8 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, bool check_constraints, bool is_internal, TransactionId frozenXid, - MultiXactId cutoffMulti) + MultiXactId cutoffMulti, + char newrelpersistence) { ObjectAddress object; Oid mapped_tables[4]; @@ -1518,6 +1520,12 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, reindex_flags = REINDEX_REL_SUPPRESS_INDEX_USE; if (check_constraints) reindex_flags |= REINDEX_REL_CHECK_CONSTRAINTS; + + if (newrelpersistence == RELPERSISTENCE_UNLOGGED) + reindex_flags |= REINDEX_REL_FORCE_UNLOGGED; + else if (newrelpersistence == RELPERSISTENCE_PERMANENT) + reindex_flags |= REINDEX_REL_FORCE_PERMANENT; + reindex_relation(OIDOldHeap, reindex_flags); /* diff --git a/src/backend/commands/indexcmds.c
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Sat, Aug 23, 2014 at 3:32 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Great. Pushed. Thanks for the patch. There is a typo in what has been pushed. Patch attached. -- Michael diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d37534e..1bb46ea 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -10722,7 +10722,7 @@ ATPrepChangeLoggedness(Relation rel, bool toLogged) /* * Scan conrelid if changing to permanent, else confrelid. This also - * determines whether an useful index exists. + * determines whether a useful index exists. */ ScanKeyInit(skey[0], toLogged ? Anum_pg_constraint_conrelid : -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Sat, Aug 23, 2014 at 8:53 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Aug 23, 2014 at 3:32 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Great. Pushed. Thanks for the patch. There is a typo in what has been pushed. Patch attached. Thanks... I fixed that in my last patch to change 'loggedness' to 'persistence'. Attached. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d37534e..1d2fe1f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -152,7 +152,7 @@ typedef struct AlteredTableInfo bool new_notnull; /* T if we added new NOT NULL constraints */ bool rewrite; /* T if a rewrite is forced */ Oid newTableSpace; /* new tablespace; 0 means no change */ - bool chgLoggedness; /* T if SET LOGGED/UNLOGGED is used */ + bool chgPersistence; /* T if SET LOGGED/UNLOGGED is used */ char newrelpersistence; /* if above is true */ /* Objects to rebuild after completing ALTER TYPE operations */ List *changedConstraintOids; /* OIDs of constraints to rebuild */ @@ -388,8 +388,8 @@ static void change_owner_recurse_to_sequences(Oid relationOid, static void ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode); static void ATExecDropCluster(Relation rel, LOCKMODE lockmode); -static bool ATPrepChangeLoggedness(Relation rel, bool toLogged); -static void ATChangeIndexesLoggedness(Oid relid, char relpersistence); +static bool ATPrepChangePersistence(Relation rel, bool toLogged); +static void ATChangeIndexesPersistence(Oid relid, char relpersistence); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename, LOCKMODE lockmode); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode); @@ -3174,19 +3174,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, break; case AT_SetLogged: /* SET LOGGED */ ATSimplePermissions(rel, ATT_TABLE); - tab-chgLoggedness = ATPrepChangeLoggedness(rel, true); + tab-chgPersistence = ATPrepChangePersistence(rel, true); tab-newrelpersistence = RELPERSISTENCE_PERMANENT; /* force rewrite if necessary */ - if (tab-chgLoggedness) + if (tab-chgPersistence) tab-rewrite = true; pass = AT_PASS_MISC; break; case AT_SetUnLogged: /* SET UNLOGGED */ ATSimplePermissions(rel, ATT_TABLE); - tab-chgLoggedness = ATPrepChangeLoggedness(rel, false); + tab-chgPersistence = ATPrepChangePersistence(rel, false); tab-newrelpersistence = RELPERSISTENCE_UNLOGGED; /* force rewrite if necessary */ - if (tab-chgLoggedness) + if (tab-chgPersistence) tab-rewrite = true; pass = AT_PASS_MISC; break; @@ -3668,7 +3668,7 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode) * Select persistence of transient table (same as original unless * user requested a change) */ - persistence = tab-chgLoggedness ? + persistence = tab-chgPersistence ? tab-newrelpersistence : OldHeap-rd_rel-relpersistence; heap_close(OldHeap, NoLock); @@ -3705,8 +3705,8 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode) * because the rewrite step might read the indexes, and that would * cause buffers for them to have the wrong setting. */ - if (tab-chgLoggedness) -ATChangeIndexesLoggedness(tab-relid, tab-newrelpersistence); + if (tab-chgPersistence) +ATChangeIndexesPersistence(tab-relid, tab-newrelpersistence); /* * Swap the physical files of the old and new heaps, then rebuild @@ -4119,7 +4119,7 @@ ATGetQueueEntry(List **wqueue, Relation rel) tab-relkind = rel-rd_rel-relkind; tab-oldDesc = CreateTupleDescCopy(RelationGetDescr(rel)); tab-newrelpersistence = RELPERSISTENCE_PERMANENT; - tab-chgLoggedness = false; + tab-chgPersistence = false; *wqueue = lappend(*wqueue, tab); @@ -10678,7 +10678,7 @@ ATExecGenericOptions(Relation rel, List *options) * checks are skipped), otherwise true. */ static bool -ATPrepChangeLoggedness(Relation rel, bool toLogged) +ATPrepChangePersistence(Relation rel, bool toLogged) { Relation pg_constraint; HeapTuple tuple; @@ -10722,7 +10722,7 @@ ATPrepChangeLoggedness(Relation rel, bool toLogged) /* * Scan conrelid if changing to permanent, else confrelid. This also - * determines whether an useful index exists. + * determines whether a useful index exists. */ ScanKeyInit(skey[0], toLogged ? Anum_pg_constraint_conrelid : @@ -10792,7 +10792,7 @@ ATPrepChangeLoggedness(Relation rel, bool toLogged) * given persistence. */ static void -ATChangeIndexesLoggedness(Oid relid, char relpersistence) +ATChangeIndexesPersistence(Oid relid, char relpersistence) {
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Fabrízio de Royes Mello wrote: Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera alvhe...@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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Fri, Aug 22, 2014 at 3:32 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fabrízio de Royes Mello wrote: Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera alvhe...@2ndquadrant.com escreveu: Fabrízio de Royes Mello wrote: I forgot to mention... I did again a lot of tests using different replication scenarios to make sure all is ok: - slaves async - slaves sync - cascade slaves On v13 you mean? Exactly! Great. Pushed. Thanks for the patch. Awesome!!! Actually I should say thank you my friend!! See you in Campinas... Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Fri, Aug 22, 2014 at 2:32 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fabrízio de Royes Mello wrote: Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Fri, Aug 22, 2014 at 4:22 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Aug 22, 2014 at 2:32 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fabrízio de Royes Mello wrote: Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera alvhe...@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. I changed to Persistence... 2. The patch seems to think that it can sometimes be safe to change the relpersistence of an existing relation. Unless you can be sure that no buffers can possibly be present in shared_buffers and nobody will use an existing relcache entry to read a new one in, it's not, because the buffers won't have the right BM_PERSISTENT marking. I'm very nervous about the fact that this patch seems not to have touched bufmgr.c, but maybe I'm missing something. Because of this concern I implement a solution pointed by Andres [1]. Regards, [1] http://www.postgresql.org/message-id/20140717230220.gk21...@awork2.anarazel.de -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d37534e..5a233e2 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -152,7 +152,7 @@ typedef struct AlteredTableInfo bool new_notnull; /* T if we added new NOT NULL constraints */ bool rewrite; /* T if a rewrite is forced */ Oid newTableSpace; /* new tablespace; 0 means no change */ - bool chgLoggedness; /* T if SET LOGGED/UNLOGGED is used */ + bool chgPersistence; /* T if SET LOGGED/UNLOGGED is used */ char newrelpersistence; /* if above is true */ /* Objects to rebuild after completing ALTER TYPE operations */ List *changedConstraintOids; /* OIDs of constraints to rebuild */ @@ -388,8 +388,8 @@ static void change_owner_recurse_to_sequences(Oid relationOid, static void ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode); static void ATExecDropCluster(Relation rel, LOCKMODE lockmode); -static bool ATPrepChangeLoggedness(Relation rel, bool toLogged); -static void ATChangeIndexesLoggedness(Oid relid, char relpersistence); +static bool ATPrepChangePersistence(Relation rel, bool toLogged); +static void ATChangeIndexesPersistence(Oid relid, char relpersistence); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, char *tablespacename, LOCKMODE lockmode); static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode); @@ -3174,19 +3174,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, break; case AT_SetLogged: /* SET LOGGED */ ATSimplePermissions(rel, ATT_TABLE); - tab-chgLoggedness = ATPrepChangeLoggedness(rel, true); + tab-chgPersistence = ATPrepChangePersistence(rel, true); tab-newrelpersistence = RELPERSISTENCE_PERMANENT; /* force rewrite if necessary */ - if (tab-chgLoggedness) + if (tab-chgPersistence) tab-rewrite = true; pass = AT_PASS_MISC; break; case AT_SetUnLogged: /* SET UNLOGGED */ ATSimplePermissions(rel, ATT_TABLE); - tab-chgLoggedness = ATPrepChangeLoggedness(rel, false); + tab-chgPersistence = ATPrepChangePersistence(rel, false); tab-newrelpersistence = RELPERSISTENCE_UNLOGGED; /* force rewrite if necessary */ - if (tab-chgLoggedness) + if (tab-chgPersistence) tab-rewrite = true; pass = AT_PASS_MISC; break; @@ -3668,7 +3668,7 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode) * Select persistence of transient table (same as original unless * user requested a change) */ - persistence = tab-chgLoggedness ? + persistence = tab-chgPersistence ? tab-newrelpersistence : OldHeap-rd_rel-relpersistence; heap_close(OldHeap, NoLock); @@ -3705,8 +3705,8 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode) * because the rewrite step might read the indexes, and that would * cause buffers for them to have the wrong setting. */ - if (tab-chgLoggedness) -ATChangeIndexesLoggedness(tab-relid, tab-newrelpersistence); + if (tab-chgPersistence) +ATChangeIndexesPersistence(tab-relid, tab-newrelpersistence); /* * Swap the physical files of the old and new heaps, then rebuild @@ -4119,7 +4119,7 @@ ATGetQueueEntry(List **wqueue, Relation rel) tab-relkind = rel-rd_rel-relkind; tab-oldDesc =
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Fabrízio de Royes Mello wrote: On Fri, Aug 22, 2014 at 4:22 PM, Robert Haas robertmh...@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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 2. The patch seems to think that it can sometimes be safe to change the relpersistence of an existing relation. Unless you can be sure that no buffers can possibly be present in shared_buffers and nobody will use an existing relcache entry to read a new one in, it's not, because the buffers won't have the right BM_PERSISTENT marking. I'm very nervous about the fact that this patch seems not to have touched bufmgr.c, but maybe I'm missing something. Right. Andres pointed this out previously, and the patch was updated consequently. The only remaining case in which we do that, again AFAIR, is for indexes, just after the table has been rewritten and just before the indexes are reindexed. There should be no buffer access of the old indexes at that point, so there would be no buffer marked with the wrong flag. Also, the table is locked access-exclusively (is that a word?), so no other transaction could possibly be reading buffers for its indexes. I pointed out, in the email just before pushing the patch, that perhaps we should pass down the new relpersistence flag into finish_heap_swap, and from there we could pass it down to reindex_index() which is where it would be needed. I'm not sure it's worth the trouble, but I think we can still ask Fabrizio to rework that part. Maybe it is me missing something. BTW why is it that index_build() checks the heap's relpersistence flag rather than the index'? I can rework this part if it's a real concern. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Fabrízio de Royes Mello wrote: On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera alvhe...@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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Fri, Aug 22, 2014 at 4:45 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: BTW why is it that index_build() checks the heap's relpersistence flag rather than the index'? I'm curious about it too... the code in src/backend/catalog/index.c is: 1975 if (heapRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED 1976 !smgrexists(indexRelation-rd_smgr, INIT_FORKNUM)) 1977 { Should not to be in that way? 1975 if (indexRelation-rd_rel-relpersistence == RELPERSISTENCE_UNLOGGED 1976 !smgrexists(indexRelation-rd_smgr, INIT_FORKNUM)) 1977 { Alvaro, is this your concern? Right? Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Re: Thom Brown 2014-08-20 CAA-aLv7TeF8iM=7u7tsgl4s5jh1a+shq_ny7gorzc_g_yj7...@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 -- c...@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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Thu, Aug 21, 2014 at 5:23 AM, Christoph Berg c...@df7cb.de wrote: Re: Thom Brown 2014-08-20 CAA-aLv7TeF8iM= 7u7tsgl4s5jh1a+shq_ny7gorzc_g_yj7...@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 -- c...@df7cb.de | http://www.df7cb.de/ Thom, Christoph is right... make no sense the message... see the example: fabrizio=# create temp table foo(); CREATE TABLE fabrizio=# alter table foo set unlogged; ERROR: table foo is unlogged The previous message is better: fabrizio=# create temp table foo(); CREATE TABLE fabrizio=# alter table foo set unlogged; ERROR: table foo is not permanent fabrizio=# fabrizio=# create unlogged table foo2(); CREATE TABLE fabrizio=# alter table foo2 set unlogged; ERROR: table foo2 is not permanent Patch attached. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..397b4e6 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -59,16 +59,17 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_name/replaceable SET WITHOUT CLUSTER +SET {LOGGED | UNLOGGED} SET WITH OIDS SET WITHOUT OIDS SET ( replaceable class=PARAMETERstorage_parameter/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] ) +SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) INHERIT replaceable class=PARAMETERparent_table/replaceable NO INHERIT replaceable class=PARAMETERparent_table/replaceable OF replaceable class=PARAMETERtype_name/replaceable NOT OF OWNER TO replaceable class=PARAMETERnew_owner/replaceable -SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable REPLICA IDENTITY {DEFAULT | USING INDEX replaceable class=PARAMETERindex_name/replaceable | FULL | NOTHING} phraseand replaceable class=PARAMETERtable_constraint_using_index/replaceable is:/phrase @@ -447,6 +448,20 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable /varlistentry varlistentry +termliteralSET {LOGGED | UNLOGGED}/literal/term +listitem + para + This form changes the table persistence type from unlogged to permanent or + from permanent to unlogged (see xref linkend=SQL-CREATETABLE-UNLOGGED). + /para + para + Changing the table persistence type acquires an literalACCESS EXCLUSIVE/literal lock + and rewrites the table contents and associated indexes into new disk files. + /para +/listitem + /varlistentry + + varlistentry termliteralSET WITH OIDS/literal/term listitem para diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index b1c411a..7f497c7 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -574,7 +574,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) heap_close(OldHeap, NoLock); /* Create the transient table that will receive the re-ordered data */ - OIDNewHeap = make_new_heap(tableOid, tableSpace, false, + OIDNewHeap = make_new_heap(tableOid, tableSpace, + OldHeap-rd_rel-relpersistence, AccessExclusiveLock); /* Copy the heap data into the new table in the desired order */ @@ -601,7 +602,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) * data, then call finish_heap_swap to complete the operation. */ Oid -make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, +make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence, LOCKMODE lockmode) { TupleDesc OldHeapDesc; @@ -613,7 +614,6 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, Datum reloptions; bool isNull; Oid namespaceid; - char relpersistence; OldHeap = heap_open(OIDOldHeap, lockmode); OldHeapDesc = RelationGetDescr(OldHeap); @@ -636,16 +636,10 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, if (isNull) reloptions = (Datum) 0; - if (forcetemp) - { + if (relpersistence == RELPERSISTENCE_TEMP) namespaceid = LookupCreationNamespace(pg_temp); - relpersistence = RELPERSISTENCE_TEMP; - } else - { namespaceid =
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On 21 August 2014 14:45, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Aug 21, 2014 at 5:23 AM, Christoph Berg c...@df7cb.de wrote: Re: Thom Brown 2014-08-20 CAA-aLv7TeF8iM= 7u7tsgl4s5jh1a+shq_ny7gorzc_g_yj7...@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 -- c...@df7cb.de | http://www.df7cb.de/ Thom, Christoph is right... make no sense the message... see the example: fabrizio=# create temp table foo(); CREATE TABLE fabrizio=# alter table foo set unlogged; ERROR: table foo is unlogged The previous message is better: fabrizio=# create temp table foo(); CREATE TABLE fabrizio=# alter table foo set unlogged; ERROR: table foo is not permanent fabrizio=# fabrizio=# create unlogged table foo2(); CREATE TABLE fabrizio=# alter table foo2 set unlogged; ERROR: table foo2 is not permanent 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. -- Thom
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On 08/21/2014 05:04 PM, Thom Brown wrote: On 21 August 2014 14:45, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Aug 21, 2014 at 5:23 AM, Christoph Berg c...@df7cb.de wrote: Re: Thom Brown 2014-08-20 CAA-aLv7TeF8iM= 7u7tsgl4s5jh1a+shq_ny7gorzc_g_yj7...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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; 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 %s to 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 termliteralSET {LOGGED | UNLOGGED}/literal/term listitem para This form changes the table 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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Tom Lane wrote: Alvaro Herrera alvhe...@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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services *** a/doc/src/sgml/ref/alter_table.sgml --- b/doc/src/sgml/ref/alter_table.sgml *** *** 61,66 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable --- 61,68 SET WITHOUT CLUSTER SET WITH OIDS SET WITHOUT OIDS + SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable + SET {LOGGED | UNLOGGED} SET ( replaceable class=PARAMETERstorage_parameter/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] ) RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) INHERIT replaceable class=PARAMETERparent_table/replaceable *** *** 68,74 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable OF replaceable class=PARAMETERtype_name/replaceable NOT OF OWNER TO replaceable class=PARAMETERnew_owner/replaceable - SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable REPLICA IDENTITY {DEFAULT | USING INDEX replaceable class=PARAMETERindex_name/replaceable | FULL | NOTHING} phraseand replaceable class=PARAMETERtable_constraint_using_index/replaceable is:/phrase --- 70,75 *** *** 477,482 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable --- 478,508 /varlistentry varlistentry + termliteralSET TABLESPACE/literal/term + listitem + para + This form changes the table's tablespace to the specified tablespace and + moves the data file(s) associated with the table to the new tablespace. + Indexes on the table, if any, are not moved; but they can be moved + separately with additional literalSET TABLESPACE/literal commands. + See also + xref linkend=SQL-CREATETABLESPACE. + /para + /listitem +/varlistentry + +varlistentry + termliteralSET {LOGGED | UNLOGGED}/literal/term + listitem + para + This form changes the table from unlogged to logged or vice-versa + (see xref linkend=SQL-CREATETABLE-UNLOGGED). It cannot be applied + to a temporary table. + /para + /listitem +/varlistentry + +varlistentry termliteralSET ( replaceable class=PARAMETERstorage_parameter/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] )/literal/term listitem para *** *** 589,608 ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable /listitem /varlistentry -varlistentry - termliteralSET TABLESPACE/literal/term - listitem - para - This form changes the table's tablespace to the specified tablespace and - moves the data file(s) associated with the table to the new
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Thu, Aug 21, 2014 at 8:04 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 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. Ok. Another option would be to pass the new relpersistence down to finish_heap_swap, but that would be hugely complicated AFAICS. I think isn't so complicated to do it, but will this improve something ? Maybe I didn't understand it very well. IMHO it just complicate a simple thing. Here's the updated patch. Thanks Alvaro! Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Thu, Aug 21, 2014 at 10:26 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Aug 21, 2014 at 8:04 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 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. Ok. Another option would be to pass the new relpersistence down to finish_heap_swap, but that would be hugely complicated AFAICS. I think isn't so complicated to do it, but will this improve something ? Maybe I didn't understand it very well. IMHO it just complicate a simple thing. Here's the updated patch. Thanks Alvaro! 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 Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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 Herrerahttp://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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Em sexta-feira, 22 de agosto de 2014, Alvaro Herrera alvhe...@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! Fabrízio Mello -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On 17 August 2014 21:45, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Mon, Jul 28, 2014 at 2:24 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Mon, Jul 28, 2014 at 1:41 PM, Christoph Berg c...@df7cb.de wrote: Re: Fabrízio de Royes Mello 2014-07-28 CAFcNs+pctx4Q2UYsLOvVFWaznO3U0XhPpkMx5DRhR=jw8w3...@mail.gmail.com There are something that should I do on this patch yet? I haven't got around to have a look at the newest incarnation yet, but I plan to do that soonish. (Of course that shouldn't stop others from doing that as well if they wish.) Thanks! Updated version. Hi Fabrizio, + This form changes the table persistence type from unlogged to permanent or + from unlogged to permanent (see xref linkend=SQL-CREATETABLE-UNLOGGED). Shouldn't this read unlogged to permanent or from permanent to unlogged? 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. Gave the patch a quick test-drive on a replicated instance, and it appears to operate as described. -- Thom
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Wed, Aug 20, 2014 at 12:35 PM, Thom Brown t...@linux.com wrote: Hi Fabrizio, + This form changes the table persistence type from unlogged to permanent or + from unlogged to permanent (see xref linkend=SQL-CREATETABLE-UNLOGGED). Shouldn't this read unlogged to permanent or from permanent to unlogged? Fixed. ERROR: table test is not permanent Perhaps this would be better as table test is unlogged as permanent doesn't match the term used in the DDL syntax. Fixed. Gave the patch a quick test-drive on a replicated instance, and it appears to operate as described. Thanks for your review. Att, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..397b4e6 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -59,16 +59,17 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_name/replaceable SET WITHOUT CLUSTER +SET {LOGGED | UNLOGGED} SET WITH OIDS SET WITHOUT OIDS SET ( replaceable class=PARAMETERstorage_parameter/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] ) +SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) INHERIT replaceable class=PARAMETERparent_table/replaceable NO INHERIT replaceable class=PARAMETERparent_table/replaceable OF replaceable class=PARAMETERtype_name/replaceable NOT OF OWNER TO replaceable class=PARAMETERnew_owner/replaceable -SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable REPLICA IDENTITY {DEFAULT | USING INDEX replaceable class=PARAMETERindex_name/replaceable | FULL | NOTHING} phraseand replaceable class=PARAMETERtable_constraint_using_index/replaceable is:/phrase @@ -447,6 +448,20 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable /varlistentry varlistentry +termliteralSET {LOGGED | UNLOGGED}/literal/term +listitem + para + This form changes the table persistence type from unlogged to permanent or + from permanent to unlogged (see xref linkend=SQL-CREATETABLE-UNLOGGED). + /para + para + Changing the table persistence type acquires an literalACCESS EXCLUSIVE/literal lock + and rewrites the table contents and associated indexes into new disk files. + /para +/listitem + /varlistentry + + varlistentry termliteralSET WITH OIDS/literal/term listitem para diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index b1c411a..7f497c7 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -574,7 +574,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) heap_close(OldHeap, NoLock); /* Create the transient table that will receive the re-ordered data */ - OIDNewHeap = make_new_heap(tableOid, tableSpace, false, + OIDNewHeap = make_new_heap(tableOid, tableSpace, + OldHeap-rd_rel-relpersistence, AccessExclusiveLock); /* Copy the heap data into the new table in the desired order */ @@ -601,7 +602,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) * data, then call finish_heap_swap to complete the operation. */ Oid -make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, +make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence, LOCKMODE lockmode) { TupleDesc OldHeapDesc; @@ -613,7 +614,6 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, Datum reloptions; bool isNull; Oid namespaceid; - char relpersistence; OldHeap = heap_open(OIDOldHeap, lockmode); OldHeapDesc = RelationGetDescr(OldHeap); @@ -636,16 +636,10 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, if (isNull) reloptions = (Datum) 0; - if (forcetemp) - { + if (relpersistence == RELPERSISTENCE_TEMP) namespaceid = LookupCreationNamespace(pg_temp); - relpersistence = RELPERSISTENCE_TEMP; - } else - { namespaceid = RelationGetNamespace(OldHeap); - relpersistence = OldHeap-rd_rel-relpersistence; - } /* * Create the new heap, using a temporary name in the same namespace as @@ -1146,6 +1140,7 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, Oid relfilenode1, relfilenode2; Oid swaptemp; + char swaprelpersistence; CatalogIndexState indstate; /* We need writable copies of both pg_class tuples. */ @@ -1177,6 +1172,10 @@ swap_relation_files(Oid r1, Oid r2, bool
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Mon, Jul 28, 2014 at 2:24 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Mon, Jul 28, 2014 at 1:41 PM, Christoph Berg c...@df7cb.de wrote: Re: Fabrízio de Royes Mello 2014-07-28 CAFcNs+pctx4Q2UYsLOvVFWaznO3U0XhPpkMx5DRhR=jw8w3...@mail.gmail.com There are something that should I do on this patch yet? I haven't got around to have a look at the newest incarnation yet, but I plan to do that soonish. (Of course that shouldn't stop others from doing that as well if they wish.) Thanks! Updated version. -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..2d131df 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -59,16 +59,17 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_name/replaceable SET WITHOUT CLUSTER +SET {LOGGED | UNLOGGED} SET WITH OIDS SET WITHOUT OIDS SET ( replaceable class=PARAMETERstorage_parameter/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] ) +SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) INHERIT replaceable class=PARAMETERparent_table/replaceable NO INHERIT replaceable class=PARAMETERparent_table/replaceable OF replaceable class=PARAMETERtype_name/replaceable NOT OF OWNER TO replaceable class=PARAMETERnew_owner/replaceable -SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable REPLICA IDENTITY {DEFAULT | USING INDEX replaceable class=PARAMETERindex_name/replaceable | FULL | NOTHING} phraseand replaceable class=PARAMETERtable_constraint_using_index/replaceable is:/phrase @@ -447,6 +448,20 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable /varlistentry varlistentry +termliteralSET {LOGGED | UNLOGGED}/literal/term +listitem + para + This form changes the table persistence type from unlogged to permanent or + from unlogged to permanent (see xref linkend=SQL-CREATETABLE-UNLOGGED). + /para + para + Changing the table persistence type acquires an literalACCESS EXCLUSIVE/literal lock + and rewrites the table contents and associated indexes into new disk files. + /para +/listitem + /varlistentry + + varlistentry termliteralSET WITH OIDS/literal/term listitem para diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index b1c411a..7f497c7 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -574,7 +574,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) heap_close(OldHeap, NoLock); /* Create the transient table that will receive the re-ordered data */ - OIDNewHeap = make_new_heap(tableOid, tableSpace, false, + OIDNewHeap = make_new_heap(tableOid, tableSpace, + OldHeap-rd_rel-relpersistence, AccessExclusiveLock); /* Copy the heap data into the new table in the desired order */ @@ -601,7 +602,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) * data, then call finish_heap_swap to complete the operation. */ Oid -make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, +make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence, LOCKMODE lockmode) { TupleDesc OldHeapDesc; @@ -613,7 +614,6 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, Datum reloptions; bool isNull; Oid namespaceid; - char relpersistence; OldHeap = heap_open(OIDOldHeap, lockmode); OldHeapDesc = RelationGetDescr(OldHeap); @@ -636,16 +636,10 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, if (isNull) reloptions = (Datum) 0; - if (forcetemp) - { + if (relpersistence == RELPERSISTENCE_TEMP) namespaceid = LookupCreationNamespace(pg_temp); - relpersistence = RELPERSISTENCE_TEMP; - } else - { namespaceid = RelationGetNamespace(OldHeap); - relpersistence = OldHeap-rd_rel-relpersistence; - } /* * Create the new heap, using a temporary name in the same namespace as @@ -1146,6 +1140,7 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, Oid relfilenode1, relfilenode2; Oid swaptemp; + char swaprelpersistence; CatalogIndexState indstate; /* We need writable copies of both pg_class tuples. */ @@ -1177,6 +1172,10 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relform1-reltablespace = relform2-reltablespace; relform2-reltablespace = swaptemp; +
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Wed, Jul 23, 2014 at 5:48 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Jul 22, 2014 at 3:29 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Jul 22, 2014 at 12:01 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund and...@2ndquadrant.com wrote: That means should I FlushRelationBuffers(rel) before change the relpersistence? I don't think that'd help. I think what this means that you simply cannot change the relpersistence of the old relation before the heap swap is successful. So I guess it has to be something like (pseudocode): OIDNewHeap = make_new_heap(..); newrel = heap_open(OIDNewHeap, AEL); /* * Change the temporary relation to be unlogged/logged. We have to do * that here so buffers for the new relfilenode will have the right * persistency set while the original filenode's buffers won't get read * in with the wrong (i.e. new) persistency setting. Otherwise a * rollback after the rewrite would possibly result with buffers for the * original filenode having the wrong persistency setting. * * NB: This relies on swap_relation_files() also swapping the * persistency. That wouldn't work for pg_class, but that can't be * unlogged anyway. */ AlterTableChangeCatalogToLoggedOrUnlogged(newrel); FlushRelationBuffers(newrel); /* copy heap data into newrel */ finish_heap_swap(); And then in swap_relation_files() also copy the persistency. That's the best I can come up right now at least. Isn't better if we can set the relpersistence as an argument to make_new_heap ? I'm thinking to change the make_new_heap: From: make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, LOCKMODE lockmode) To: make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence, LOCKMODE lockmode) That way we can create the new heap with the appropriate relpersistence, so in the swap_relation_files also copy the persistency, of course. And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes, because in the finish_heap_swap they'll be rebuild. Thoughts? The attached patch implement my previous idea based on Andres thoughts. I don't liked the last version of the patch, especially this part: +/* check if SetUnlogged or SetLogged exists in subcmds */ +for(pass = 0; pass AT_NUM_PASSES; pass++) +{ +List *subcmds = tab-subcmds[pass]; +ListCell*lcmd; + +if (subcmds == NIL) +continue; + +foreach(lcmd, subcmds) +{ +AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd); + +if (cmd-subtype == AT_SetUnLogged || cmd-subtype == AT_SetLogged) +{ +/* + * Change the temporary relation to be unlogged/logged. We have to do + * that here so buffers for the new relfilenode will have the right + * persistency set while the original filenode's buffers won't get read + * in with the wrong (i.e. new) persistency setting. Otherwise a + * rollback after the rewrite would possibly result with buffers for the + * original filenode having the wrong persistency setting. + * + * NB: This relies on swap_relation_files() also swapping the + * persistency. That wouldn't work for pg_class, but that can't be + * unlogged anyway. + */ +if (cmd-subtype == AT_SetUnLogged) +newrelpersistence = RELPERSISTENCE_UNLOGGED; + +isSetLoggedUnlogged = true; +} +} +} So I did a refactoring adding new items to AlteredTableInfo to pass the information through the phases. Hi all, There are something that should I do on this patch yet? Regards -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Re: Fabrízio de Royes Mello 2014-07-28 CAFcNs+pctx4Q2UYsLOvVFWaznO3U0XhPpkMx5DRhR=jw8w3...@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 -- c...@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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Mon, Jul 28, 2014 at 1:41 PM, Christoph Berg c...@df7cb.de wrote: Re: Fabrízio de Royes Mello 2014-07-28 CAFcNs+pctx4Q2UYsLOvVFWaznO3U0XhPpkMx5DRhR=jw8w3...@mail.gmail.com There are something that should I do on this patch yet? I haven't got around to have a look at the newest incarnation yet, but I plan to do that soonish. (Of course that shouldn't stop others from doing that as well if they wish.) Thanks! -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Tue, Jul 22, 2014 at 3:29 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Jul 22, 2014 at 12:01 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund and...@2ndquadrant.com wrote: That means should I FlushRelationBuffers(rel) before change the relpersistence? I don't think that'd help. I think what this means that you simply cannot change the relpersistence of the old relation before the heap swap is successful. So I guess it has to be something like (pseudocode): OIDNewHeap = make_new_heap(..); newrel = heap_open(OIDNewHeap, AEL); /* * Change the temporary relation to be unlogged/logged. We have to do * that here so buffers for the new relfilenode will have the right * persistency set while the original filenode's buffers won't get read * in with the wrong (i.e. new) persistency setting. Otherwise a * rollback after the rewrite would possibly result with buffers for the * original filenode having the wrong persistency setting. * * NB: This relies on swap_relation_files() also swapping the * persistency. That wouldn't work for pg_class, but that can't be * unlogged anyway. */ AlterTableChangeCatalogToLoggedOrUnlogged(newrel); FlushRelationBuffers(newrel); /* copy heap data into newrel */ finish_heap_swap(); And then in swap_relation_files() also copy the persistency. That's the best I can come up right now at least. Isn't better if we can set the relpersistence as an argument to make_new_heap ? I'm thinking to change the make_new_heap: From: make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, LOCKMODE lockmode) To: make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence, LOCKMODE lockmode) That way we can create the new heap with the appropriate relpersistence, so in the swap_relation_files also copy the persistency, of course. And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes, because in the finish_heap_swap they'll be rebuild. Thoughts? The attached patch implement my previous idea based on Andres thoughts. I don't liked the last version of the patch, especially this part: +/* check if SetUnlogged or SetLogged exists in subcmds */ +for(pass = 0; pass AT_NUM_PASSES; pass++) +{ +List *subcmds = tab-subcmds[pass]; +ListCell*lcmd; + +if (subcmds == NIL) +continue; + +foreach(lcmd, subcmds) +{ +AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd); + +if (cmd-subtype == AT_SetUnLogged || cmd-subtype == AT_SetLogged) +{ +/* + * Change the temporary relation to be unlogged/logged. We have to do + * that here so buffers for the new relfilenode will have the right + * persistency set while the original filenode's buffers won't get read + * in with the wrong (i.e. new) persistency setting. Otherwise a + * rollback after the rewrite would possibly result with buffers for the + * original filenode having the wrong persistency setting. + * + * NB: This relies on swap_relation_files() also swapping the + * persistency. That wouldn't work for pg_class, but that can't be + * unlogged anyway. + */ +if (cmd-subtype == AT_SetUnLogged) +newrelpersistence = RELPERSISTENCE_UNLOGGED; + +isSetLoggedUnlogged = true; +} +} +} So I did a refactoring adding new items to AlteredTableInfo to pass the information through the phases. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..2d131df 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -59,16 +59,17 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_name/replaceable SET WITHOUT CLUSTER +SET {LOGGED | UNLOGGED} SET WITH OIDS SET WITHOUT OIDS SET ( replaceable class=PARAMETERstorage_parameter/replaceable =
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund and...@2ndquadrant.com wrote: That means should I FlushRelationBuffers(rel) before change the relpersistence? I don't think that'd help. I think what this means that you simply cannot change the relpersistence of the old relation before the heap swap is successful. So I guess it has to be something like (pseudocode): OIDNewHeap = make_new_heap(..); newrel = heap_open(OIDNewHeap, AEL); /* * Change the temporary relation to be unlogged/logged. We have to do * that here so buffers for the new relfilenode will have the right * persistency set while the original filenode's buffers won't get read * in with the wrong (i.e. new) persistency setting. Otherwise a * rollback after the rewrite would possibly result with buffers for the * original filenode having the wrong persistency setting. * * NB: This relies on swap_relation_files() also swapping the * persistency. That wouldn't work for pg_class, but that can't be * unlogged anyway. */ AlterTableChangeCatalogToLoggedOrUnlogged(newrel); FlushRelationBuffers(newrel); /* copy heap data into newrel */ finish_heap_swap(); And then in swap_relation_files() also copy the persistency. That's the best I can come up right now at least. Isn't better if we can set the relpersistence as an argument to make_new_heap ? I'm thinking to change the make_new_heap: From: make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, LOCKMODE lockmode) To: make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence, LOCKMODE lockmode) That way we can create the new heap with the appropriate relpersistence, so in the swap_relation_files also copy the persistency, of course. And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes, because in the finish_heap_swap they'll be rebuild. Thoughts? Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Tue, Jul 22, 2014 at 12:01 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Jul 17, 2014 at 8:02 PM, Andres Freund and...@2ndquadrant.com wrote: That means should I FlushRelationBuffers(rel) before change the relpersistence? I don't think that'd help. I think what this means that you simply cannot change the relpersistence of the old relation before the heap swap is successful. So I guess it has to be something like (pseudocode): OIDNewHeap = make_new_heap(..); newrel = heap_open(OIDNewHeap, AEL); /* * Change the temporary relation to be unlogged/logged. We have to do * that here so buffers for the new relfilenode will have the right * persistency set while the original filenode's buffers won't get read * in with the wrong (i.e. new) persistency setting. Otherwise a * rollback after the rewrite would possibly result with buffers for the * original filenode having the wrong persistency setting. * * NB: This relies on swap_relation_files() also swapping the * persistency. That wouldn't work for pg_class, but that can't be * unlogged anyway. */ AlterTableChangeCatalogToLoggedOrUnlogged(newrel); FlushRelationBuffers(newrel); /* copy heap data into newrel */ finish_heap_swap(); And then in swap_relation_files() also copy the persistency. That's the best I can come up right now at least. Isn't better if we can set the relpersistence as an argument to make_new_heap ? I'm thinking to change the make_new_heap: From: make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, LOCKMODE lockmode) To: make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence, LOCKMODE lockmode) That way we can create the new heap with the appropriate relpersistence, so in the swap_relation_files also copy the persistency, of course. And after copy the heap data to the new table (ATRewriteTable) change relpersistence of the OldHeap's indexes, because in the finish_heap_swap they'll be rebuild. Thoughts? The attached patch implement my previous idea based on Andres thoughts. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..2d131df 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -59,16 +59,17 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_name/replaceable SET WITHOUT CLUSTER +SET {LOGGED | UNLOGGED} SET WITH OIDS SET WITHOUT OIDS SET ( replaceable class=PARAMETERstorage_parameter/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] ) +SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) INHERIT replaceable class=PARAMETERparent_table/replaceable NO INHERIT replaceable class=PARAMETERparent_table/replaceable OF replaceable class=PARAMETERtype_name/replaceable NOT OF OWNER TO replaceable class=PARAMETERnew_owner/replaceable -SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable REPLICA IDENTITY {DEFAULT | USING INDEX replaceable class=PARAMETERindex_name/replaceable | FULL | NOTHING} phraseand replaceable class=PARAMETERtable_constraint_using_index/replaceable is:/phrase @@ -447,6 +448,20 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable /varlistentry varlistentry +termliteralSET {LOGGED | UNLOGGED}/literal/term +listitem + para + This form changes the table persistence type from unlogged to permanent or + from unlogged to permanent (see xref linkend=SQL-CREATETABLE-UNLOGGED). + /para + para + Changing the table persistence type acquires an literalACCESS EXCLUSIVE/literal lock + and rewrites the table contents and associated indexes into new disk files. + /para +/listitem + /varlistentry + + varlistentry termliteralSET WITH OIDS/literal/term listitem para diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index b1c411a..7f497c7 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -574,7 +574,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) heap_close(OldHeap, NoLock); /* Create the transient table that will receive the re-ordered data */ - OIDNewHeap = make_new_heap(tableOid, tableSpace, false, + OIDNewHeap = make_new_heap(tableOid, tableSpace, + OldHeap-rd_rel-relpersistence,
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Re: Fabrízio de Royes Mello 2014-07-16 cafcns+r_lmddxjrabwnj3rmy0qegwa-vv6v2smdescofb2d...@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=PARAMETERnew_tablespace/replaceable RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) INHERIT replaceable class=PARAMETERparent_table/replaceable NO INHERIT replaceable class=PARAMETERparent_table/replaceable OF replaceable class=PARAMETERtype_name/replaceable NOT OF OWNER TO replaceable class=PARAMETERnew_owner/replaceable -SET TABLESPACE replaceable class=PARAMETERnew_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 -- c...@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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Mon, Jul 21, 2014 at 9:51 AM, Andres Freund and...@2ndquadrant.com wrote: On 2014-07-16 20:45:15 -0300, Fabrízio de Royes Mello wrote: The rewrite will read in the 'old' contents - but because it's done after the pg_class.relpersistence is changed they'll all not be marked as BM_PERMANENT in memory. Then the ALTER TABLE is rolled back, including the relpersistence setting. Which will unfortunately leave pages with the wrong persistency setting in memory, right? That means should I FlushRelationBuffers(rel) before change the relpersistence? Did my explanation clarify the problem + possible solution sufficiently? Yes, your explanation was enough. I didn't had time to working on it yet. But I hope working on it today or tomorrow at least. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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 and...@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 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: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Re: Fabrízio de Royes Mello 2014-07-15 CAFcNs+pXpmfwi_rKF-cSBOHWC+E=xtsrnxicrgay6bcmthb...@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=PARAMETERnew_tablespace/replaceable RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) INHERIT replaceable class=PARAMETERparent_table/replaceable NO INHERIT replaceable class=PARAMETERparent_table/replaceable OF replaceable class=PARAMETERtype_name/replaceable NOT OF OWNER TO replaceable class=PARAMETERnew_owner/replaceable -SET TABLESPACE replaceable class=PARAMETERnew_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) +{ + charrelpersistence; + + 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), +
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Wed, Jul 16, 2014 at 3:53 PM, Andres Freund and...@2ndquadrant.com wrote: On 2014-07-16 20:25:42 +0200, Andres Freund wrote: Hi, I quickly looked at this patch and I think there's major missing pieces around buffer management and wal logging. a) Currently buffers that are in memory marked as permanent/non-permanent aren't forced out to disk/pruned from cache, not even when they're dirty. b) When converting from a unlogged to a logged table the relation needs to be fsynced. c) Currently a unlogged table changed into a logged one will be corrupted on a standby because its contents won't ever be WAL logged. Forget that, didn't notice that you're setting tab-rewrite = true. :-) -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Wed, Jul 16, 2014 at 7:26 PM, Andres Freund and...@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 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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Hi Fabrízio, thanks for the speedy new version. Re: Fabrízio de Royes Mello 2014-07-15 cafcns+opbuhjuo1soathv8zqcowqp-yerjfoo15v1xpxspc...@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 -- c...@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
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Tue, Jul 15, 2014 at 3:04 PM, Christoph Berg c...@df7cb.de wrote: Hi Fabrízio, thanks for the speedy new version. You're welcome... If all happen ok I would like to have this patch commited before the GSoC2014 ends. I've just tried some SET (UN)LOGGED operations with altering column types in the same operation, that works. But: Yes, you should use the existing table rewriting machinery, or at least clearly document (in comments) why it doesn't work for you. Also looking at ATController, there's a wqueue mechanism to queue catalog updates. You should probably use this, too, or again document why it doesn't work for you. This works... fixed! Thanks. What about the wqueue mechanism, though? Isn't that made exactly for the kind of catalog updates you are doing? Works, but this mechanism create a new entry in pg_class for toast, so it's a little different than use the cluster_rel that generate a new relfilenode. The important is both mechanisms create new datafiles. You miss the symmetric case the other way round. When changing a table to unlogged, you need to make sure no other permanent table is referencing our table. Ohh yeas... sorry... you're completely correct... fixed! Can you move ATPrepSetUnLogged next to ATPrepSetLogged as both reference AlterTableSetLoggedCheckForeignConstraints now, and fix the comment on ATPrepSetUnLogged to also mention FKs? I'd also reiterate my proposal to merge these into one function, given they are now doing the same checks. Merged both to a single ATPrepSetLoggedOrUnlogged(Relation rel, bool toLogged); In AlterTableSetLoggedCheckForeignConstraints, move relfk = relation_open... out of the if because it's duplicated, and also for symmetry with relation_close(). But they aren't duplicated... the first opens con-confrelid and the other opens con-conrelid according toLogged branch. The function needs comments. It is somewhat clear that self-referencing FKs will be skipped, but the two if (toLogged) branches should be annotated to say which case they are really about. Fixed. Instead of just switching the argument order in the errmsg arguments, the error text should be updated to read table %s is referenced by permanent table %s. At the moment the error text is wrong because the table logged1 is not yet unlogged: +ALTER TABLE logged1 SET UNLOGGED; +ERROR: table logged2 references unlogged table logged1 - table logged1 is referenced by permanent table logged2 Sorry... my mistake... fixed Compared to v3, you've removed a lot of SELECT t.relpersistence... from the regression tests, was that intended? I removed because they are not so useful than I was thinking before. Actually they just bloated our test cases. I think the tests could also use a bit more comments, notably the commands that are expected to fail. So far I haven't tried to read them but trusted that they did the right thing. (Though with the SELECTs removed, it's pretty readable now.) Added some comments. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..2d131df 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -59,16 +59,17 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_name/replaceable SET WITHOUT CLUSTER +SET {LOGGED | UNLOGGED} SET WITH OIDS SET WITHOUT OIDS SET ( replaceable class=PARAMETERstorage_parameter/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] ) +SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable RESET ( replaceable class=PARAMETERstorage_parameter/replaceable [, ... ] ) INHERIT replaceable class=PARAMETERparent_table/replaceable NO INHERIT replaceable class=PARAMETERparent_table/replaceable OF replaceable class=PARAMETERtype_name/replaceable NOT OF OWNER TO replaceable class=PARAMETERnew_owner/replaceable -SET TABLESPACE replaceable class=PARAMETERnew_tablespace/replaceable REPLICA IDENTITY {DEFAULT | USING INDEX replaceable class=PARAMETERindex_name/replaceable | FULL | NOTHING} phraseand replaceable class=PARAMETERtable_constraint_using_index/replaceable is:/phrase @@ -447,6 +448,20 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable /varlistentry varlistentry +termliteralSET {LOGGED | UNLOGGED}/literal/term +listitem + para + This form changes the table persistence type from unlogged to permanent or + from unlogged to permanent (see
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Re: Fabrízio de Royes Mello 2014-07-12 cafcns+qf5fukkp9vdjwokiabn_rrm4+hjqxeevpd_7-to0l...@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=PARAMETERname/replaceable ENABLE REPLICA RULE replaceable class=PARAMETERrewrite_rule_name/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_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 -- c...@df7cb.de |
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Mon, Jul 14, 2014 at 3:31 PM, Christoph Berg c...@df7cb.de wrote: Oh I wasn't aware of the wiki page, I had just read the old thread. Thanks for the pointer. :-) Thanks again for your review! diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..424f2e9 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -58,6 +58,7 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ENABLE REPLICA RULE replaceable class=PARAMETERrewrite_rule_name/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_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. 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 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 diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 69a1e14..2d131df 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -59,16 +59,17 @@ ALTER TABLE [ IF EXISTS ]
Re: [HACKERS] [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+jgvwxc8uny5jsrdwy...@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=PARAMETERname/replaceable ENABLE REPLICA RULE replaceable class=PARAMETERrewrite_rule_name/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_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=PARAMETERname/replaceable /varlistentry varlistentry +termliteralSET {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 literalACCESS 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 literalACCESS 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_INDEX2 /* re-add existing indexes */ -#define AT_PASS_OLD_CONSTR 3 /* re-add existing constraints */ -#define AT_PASS_COL_ATTRS4 /* 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_INDEX2 /* re-add existing indexes */ +#define AT_PASS_OLD_CONSTR 3 /* re-add existing constraints */ +#define AT_PASS_COL_ATTRS4 /* 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_INDEX6 /* ADD indexes */ -#define AT_PASS_ADD_CONSTR 7 /* ADD constraints, defaults */ -#define AT_PASS_MISC 8 /* other stuff */ -#define AT_NUM_PASSES9 +#define AT_PASS_ADD_COL 5 /* ADD COLUMN */ +#define AT_PASS_ADD_INDEX6 /* ADD indexes */ +#define AT_PASS_ADD_CONSTR 7 /* ADD constraints, defaults */ +#define AT_PASS_MISC 8 /* other stuff */
Re: [HACKERS] [GSoC2014] Patch ALTER TABLE ... SET LOGGED
On Wed, Jun 11, 2014 at 1:19 PM, Fabrízio de Royes Mello fabriziome...@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 -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello 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=PARAMETERname/replaceable ENABLE REPLICA RULE replaceable class=PARAMETERrewrite_rule_name/replaceable ENABLE ALWAYS RULE replaceable class=PARAMETERrewrite_rule_name/replaceable CLUSTER ON replaceable class=PARAMETERindex_name/replaceable +SET {LOGGED | UNLOGGED} SET WITHOUT CLUSTER SET WITH OIDS SET WITHOUT OIDS @@ -432,6 +433,20 @@ ALTER TABLE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable /varlistentry varlistentry +termliteralSET {LOGGED | UNLOGGED}/literal/term +listitem + para + This form change the table persistence type from unlogged to permanent or + 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 literalACCESS EXCLUSIVE/literal lock. + /para +/listitem + /varlistentry + + varlistentry termliteralSET WITHOUT CLUSTER/literal/term listitem 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_DROP0 /* 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_COL5 /* ADD COLUMN */ +#define AT_PASS_ADD_INDEX 6 /* ADD indexes */ +#define AT_PASS_ADD_CONSTR 7 /* ADD constraints, defaults */ +#define AT_PASS_MISC8 /* other stuff */ +#define AT_PASS_SET_LOGGED_UNLOGGED 9 /* SET LOGGED and UNLOGGED */ +#define AT_NUM_PASSES10 typedef struct AlteredTableInfo { @@ -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); 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); static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,