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

On Thu, Nov 6, 2014 at 3:42 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
> On Sat, Sep 13, 2014 at 11:02 PM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)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