Re: [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 <
fabriziomello(at)gmail(dot)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
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