From: | Andres Freund <andres(at)2ndquadrant(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Peter Eisentraut <peter_e(at)gmx(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Simon Riggs <simon(at)2ndquadrant(dot)com> |
Subject: | Re: Support for REINDEX CONCURRENTLY |
Date: | 2013-06-24 10:39:37 |
Message-ID: | 20130624103937.GB6471@awork2.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2013-06-24 07:46:34 +0900, Michael Paquier wrote:
> On Mon, Jun 24, 2013 at 7:22 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > Compile error ;)
> It looks like filterdiff did not work correctly when generating the
> latest patch with context diffs, I cannot apply it cleanly wither.
> This is perhaps due to a wrong manipulation from me. Please try the
> attached that has been generated as a raw git output. It works
> correctly with a git apply. I just checked.
Did you check whether that introduces a performance regression?
> /* ----------
> + * toast_get_valid_index
> + *
> + * Get the valid index of given toast relation. A toast relation can only
> + * have one valid index at the same time. The lock taken on the index
> + * relations is released at the end of this function call.
> + */
> +Oid
> +toast_get_valid_index(Oid toastoid, LOCKMODE lock)
> +{
> + ListCell *lc;
> + List *indexlist;
> + int num_indexes, i = 0;
> + Oid validIndexOid;
> + Relation validIndexRel;
> + Relation *toastidxs;
> + Relation toastrel;
> +
> + /* Get the index list of relation */
> + toastrel = heap_open(toastoid, lock);
> + indexlist = RelationGetIndexList(toastrel);
> + num_indexes = list_length(indexlist);
> +
> + /* Open all the index relations */
> + toastidxs = (Relation *) palloc(num_indexes * sizeof(Relation));
> + foreach(lc, indexlist)
> + toastidxs[i++] = index_open(lfirst_oid(lc), lock);
> +
> + /* Fetch valid toast index */
> + validIndexRel = toast_index_fetch_valid(toastidxs, num_indexes);
> + validIndexOid = RelationGetRelid(validIndexRel);
> +
> + /* Close all the index relations */
> + for (i = 0; i < num_indexes; i++)
> + index_close(toastidxs[i], lock);
> + pfree(toastidxs);
> + list_free(indexlist);
> +
> + heap_close(toastrel, lock);
> + return validIndexOid;
> +}
Just to make sure, could you check we've found a valid index?
> static bool
> -toastrel_valueid_exists(Relation toastrel, Oid valueid)
> +toastrel_valueid_exists(Relation toastrel, Oid valueid, LOCKMODE lockmode)
> {
> bool result = false;
> ScanKeyData toastkey;
> SysScanDesc toastscan;
> + int i = 0;
> + int num_indexes;
> + Relation *toastidxs;
> + Relation validtoastidx;
> + ListCell *lc;
> + List *indexlist;
> +
> + /* Ensure that the list of indexes of toast relation is computed */
> + indexlist = RelationGetIndexList(toastrel);
> + num_indexes = list_length(indexlist);
> +
> + /* Open each index relation necessary */
> + toastidxs = (Relation *) palloc(num_indexes * sizeof(Relation));
> + foreach(lc, indexlist)
> + toastidxs[i++] = index_open(lfirst_oid(lc), lockmode);
> +
> + /* Fetch a valid index relation */
> + validtoastidx = toast_index_fetch_valid(toastidxs, num_indexes);
Those 10 lines are repeated multiple times, in different
functions. Maybe move them into toast_index_fetch_valid and rename that
to
Relation *
toast_open_indexes(Relation toastrel, LOCKMODE mode, size_t *numindexes, size_t valididx);
That way we also wouldn't fetch/copy the indexlist twice in some
functions.
> + /* Clean up */
> + for (i = 0; i < num_indexes; i++)
> + index_close(toastidxs[i], lockmode);
> + list_free(indexlist);
> + pfree(toastidxs);
The indexlist could already be freed inside the function proposed
above...
> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> index 8294b29..2b777da 100644
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -8782,7 +8783,13 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
> errmsg("cannot move temporary tables of other sessions")));
>
> + foreach(lc, reltoastidxids)
> + {
> + Oid toastidxid = lfirst_oid(lc);
> + if (OidIsValid(toastidxid))
> + ATExecSetTableSpace(toastidxid, newTableSpace, lockmode);
> + }
Copy & pasted OidIsValid(), shouldn't be necessary anymore.
Otherwise I think there's not really much left to be done. Fujii?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Albe Laurenz | 2013-06-24 11:00:14 | Re: [HACKERS] Frontend/backend protocol improvements proposal (request). |
Previous Message | Cédric Villemain | 2013-06-24 10:36:59 | Re: [Review] Re: minor patch submission: CREATE CAST ... AS EXPLICIT |