Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED
От | Michael Paquier |
---|---|
Тема | Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED |
Дата | |
Msg-id | CAB7nPqQB_Kn-uCO6bzsNgPXXNyCxLdvDX6+HFnbf4BsFRtDs6g@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED (Fabrízio de Royes Mello <fabriziomello@gmail.com>) |
Ответы |
Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED
|
Список | pgsql-hackers |
Thanks for the updated patch, Fabrizio.
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.
On Tue, Nov 11, 2014 at 7:44 AM, Fabrízio de Royes Mello <fabriziomello@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+9bjoLNxiO47QSrO3HCBSoQ0qFA@mail.gmail.com
http://www.postgresql.org/message-id/CAB7nPqSEB+HwiTXfWKQyPA7+9bjoLNxiO47QSrO3HCBSoQ0qFA@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)
- 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
Вложения
В списке pgsql-hackers по дате отправления: