On Thu, Nov 6, 2014 at 3:42 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > > On Sat, Sep 13, 2014 at 11:02 PM, Fabrízio de Royes Mello > <fabriziomello@gmail.com> wrote: > > Patch rebased and added to commitfest [1]. > It looks like a good thing to remove ATChangeIndexesPersistence, this > puts the persistence switch directly into reindex process. > > A couple of minor comments about this patch: > 1) Reading it, I am wondering if it would not be finally time to > switch to a macro to get a relation's persistence, something like > RelationGetPersistence in rel.h... Not related directly to this patch.
Good idea... I'll provide a patch soon.
> 2) reindex_index has as new argument a relpersislence value for the > new index. reindex_relation has differently a new set of flags to > enforce the relpersistence of all the underling indexes. Wouldn't it > be better for API consistency to pass directly a relpersistence value > through reindex_relation? In any case, the comment block of > reindex_relation does not contain a description of the new flags.
I did it because the ReindexDatabase build a list of oids to run reindex_relation for each item of the list. I can change the list to store the relpersistence also, but this can lead us to a concurrency trouble because if one session execute REINDEX DATABASE and other session run "ALTER TABLE name SET {LOGGED|UNLOGGED}" during the building of the list the reindex can lead us to a inconsitence state.
Added comments to comment block of reindex_relation.
> 3) Here you may as well just set the value and be done: > + /* > + * Check if need to set the new relpersistence > + */ > + if (iRel->rd_rel->relpersistence != relpersistence) > + iRel->rd_rel->relpersistence = relpersistence;
Hmmm... I really don't know why I did it... fixed.
