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

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.
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