Re: Support for REINDEX CONCURRENTLY

On 2013-02-07 17:28:53 +0900, Michael Paquier wrote:
> On Thu, Feb 7, 2013 at 5:15 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > On 2013-02-07 03:01:36 -0500, Tom Lane wrote:
> > > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > > > What about
> > >
> > > > 3) Use reltoastidxid if != InvalidOid and manually build the list
> > (using
> > > > RelationGetIndexList) otherwise?
> > >
> > > Do we actually need reltoastidxid at all? I always thought having that
> > > field was a case of premature optimization.
> >
> > I am a bit doubtful its really measurable as well. Really supporting a
> > dynamic number of indexes might be noticeable because we would need to
> > allocate memory et al for each toasted Datum, but only supporting one or
> > two seems easy enough.
> >
> > The only advantage besides the dubious performance advantage of my
> > proposed solution is that less code needs to change as only
> > toast_save_datum() would need to change.
> >
> > > There might be some case
> > > for keeping it to avoid breaking any client-side code that might be
> > > looking at it ... but if you're proposing changing the field contents
> > > anyway, that argument goes right out the window.
> >
> > Well, it would only be 0/InvalidOid while being reindexed concurrently,
> > but yea.
> >
> Removing reltoastindxid is more appealing for at least 2 reasons regarding
> current implementation of REINDEX CONCURRENTLY:
> 1) if reltoastidxid is set to InvalidOid during a concurrent reindex and
> reindex fails, how would it be possible to set it back to the correct
> value? This would need more special code, which could become a maintenance
> burden for sure.

I would just let it stay slightly less efficient till the index is
dropped/reindexed.

> Btw, I think that if this optimization for toast relations is done, it
> should be a separate patch.

What do you mean by a separate patch? Commit it before committing
REINDEX CONCURRENTLY? If so, yes, sure. If you mean it can be fixed
later, I don't really see how, since this is an unresolved problem...

> Also, as I am not a specialist in toast
> indexes, any opinion about potential performance impact (if any) is welcome
> if we remove reltoastidxid and use RelationGetIndexList instead.

Tom doubted it will be really measurable, so did I... If anything I
think it will be measurable during querying toast tables. So possibly we
would have to retain reltoastidxid for querying...

The minimal (not so nice) patch to make this correct probably is fairly
easy.

Changing only toast_save_datum:

Relation toastidx[2];

...
if (toastrel->rd_indexvalid == 0)
RelationGetIndexList(toastrel);

num_indexes = list_length(toastrel->rd_indexlist);

if (num_indexes == 1)
toastidx[0] = index_open(toastrel->rd_rel->reltoastidxid);
else if (num_indexes == 2)
{
int off = 0;
ListCell *l;

foreach(l, RelationGetIndexList(toastrel))
toastidx[off] = index_open(lfirst_oid(l));
}
else
elog(ERROR, "toast indexes with unsupported number of indexes");

...
for (cur_index = 0; cur_index < num_indexes; cur_index++)
index_insert(toastidx[cur_index], t_values, t_isnull,
&(toasttup->t_self),
toastrel,
toastidx[cur_index]->rd_index->indisunique ?
UNIQUE_CHECK_YES : UNIQUE_CHECK_NO);
...
for (cur_index = 0; cur_index < num_indexes; cur_index++)
index_close(toastidx[cur_index], RowExclusiveLock);

(that indisunique check seems like a copy&paste remnant btw).

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services