Date: Thu, 2 Apr 2020 01:52:09 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Amit Langote <amitlangote09@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@postgresql.org
Subject: Re: ALTER tbl rewrite loses CLUSTER ON index
Message-ID: <20200402065209.GF14618@telsasoft.com>
References: <20200316132731.GJ26184@telsasoft.com>
 <20200316142523.GA14606@alvherre.pgsql>
 <20200317053332.GE2206@paquier.xyz>
 <20200317162044.GX26184@telsasoft.com>
 <20200318024837.GG214947@paquier.xyz>
 <20200402061421.GA112468@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200402061421.GA112468@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Apr 02, 2020 at 03:14:21PM +0900, Michael Paquier wrote:
> Now, regarding patch 0002, note that you have a problem for this part:
> -            tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
> -            if (!HeapTupleIsValid(tuple))    /* probably can't happen */
> -            {
> -                relation_close(OldHeap, AccessExclusiveLock);
> -                pgstat_progress_end_command();
> -                return;
> -            }
> -            indexForm = (Form_pg_index) GETSTRUCT(tuple);
> -            if (!indexForm->indisclustered)
> +            if (!get_index_isclustered(indexOid))
>              {
> -                ReleaseSysCache(tuple);
>                  relation_close(OldHeap, AccessExclusiveLock);
>                  pgstat_progress_end_command();
>                  return;
>              }
> -            ReleaseSysCache(tuple);
> 
> On an invalid tuple for pg_index, the new code would issue an error,
> while the old code would just return.  And it seems to me that this
> can lead to problems because the parent relation is processed and
> locked at the beginning of cluster_rel(), *after* we know the index
> OID to work on.

> The refactoring is fine for the other two areas
> though, so I think that there is still value to apply
> get_index_isclustered() within mark_index_clustered() and cluster(),
> and I would suggest to keep 0002 to that.
> 
> Justin, what do you think?

Sounds right.  Or else get_index_isclustered() could be redefined to take a
boolean "do_elog" flag, and if syscache fails and that's false, then return
false instead of ERROR.

-- 
Justin