Date: Mon, 23 Mar 2020 16:47:04 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, pgsql-hackers@postgresql.org
Subject: Re: ALTER INDEX fails on partitioned index
Message-ID: <20200323214704.GM2563@telsasoft.com>
References: <20190206173212.GA23770@alvherre.pgsql>
 <201901071923.7r2xflxvncly@alvherre.pgsql>
 <20191227035157.GD12890@telsasoft.com>
 <CA+Tgmobq8_-DS7qDEmMi-4ARP1_0bkgFEjYfiK97L2eXq+Q+nw@mail.gmail.com>
 <20200227232513.GE29456@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200227232513.GE29456@telsasoft.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Feb 27, 2020 at 09:11:14PM -0300, Alvaro Herrera wrote:
> On 2020-Feb-27, Justin Pryzby wrote:
> > The attached allows CREATE/ALTER to specify reloptions on a partitioned table
> > which are used as defaults for future children.
> > 
> > I think that's a desirable behavior, same as for tablespaces.  Michael
> > mentioned that ALTER INDEX ONLY doesn't exist, but that's only an issue if
> > ALTER acts recursively, which isn't the case here.
> 
> I think ALTER not acting recursively is a bug that we would do well not
> to propagate any further.  Enough effort we have to spend trying to fix
> that already.  Let's add ALTER .. ONLY if needed.

I was modeling after the behavior for tablespaces, and didn't realize that
non-recursive alter was considered discouraged. 

On Thu, Feb 27, 2020 at 05:25:13PM -0600, Justin Pryzby wrote:
> The first patch makes a prettier message, per Robert's suggestion.

Is there any interest in this one ?

> From e5bb363f514d768a4f540d9c82ad5745944b1486 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Mon, 30 Dec 2019 09:31:03 -0600
> Subject: [PATCH v2 1/2] More specific error message when failing to alter a
>  partitioned index..
> 
> "..is not an index" is deemed to be unfriendly
> 
> https://www.postgresql.org/message-id/CA%2BTgmobq8_-DS7qDEmMi-4ARP1_0bkgFEjYfiK97L2eXq%2BQ%2Bnw%40mail.gmail.com
> ---
>  src/backend/commands/tablecmds.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> index b7c8d66..1b271af 100644
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -366,7 +366,7 @@ static void ATRewriteTables(AlterTableStmt *parsetree,
>  static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode);
>  static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel);
>  static void ATSimplePermissions(Relation rel, int allowed_targets);
> -static void ATWrongRelkindError(Relation rel, int allowed_targets);
> +static void ATWrongRelkindError(Relation rel, int allowed_targets, int actual_target);
>  static void ATSimpleRecursion(List **wqueue, Relation rel,
>  							  AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode,
>  							  AlterTableUtilityContext *context);
> @@ -5458,7 +5458,7 @@ ATSimplePermissions(Relation rel, int allowed_targets)
>  
>  	/* Wrong target type? */
>  	if ((actual_target & allowed_targets) == 0)
> -		ATWrongRelkindError(rel, allowed_targets);
> +		ATWrongRelkindError(rel, allowed_targets, actual_target);
>  
>  	/* Permissions checks */
>  	if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
> @@ -5479,7 +5479,7 @@ ATSimplePermissions(Relation rel, int allowed_targets)
>   * type.
>   */
>  static void
> -ATWrongRelkindError(Relation rel, int allowed_targets)
> +ATWrongRelkindError(Relation rel, int allowed_targets, int actual_target)
>  {
>  	char	   *msg;
>  
> @@ -5527,9 +5527,20 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
>  			break;
>  	}
>  
> -	ereport(ERROR,
> -			(errcode(ERRCODE_WRONG_OBJECT_TYPE),
> -			 errmsg(msg, RelationGetRelationName(rel))));
> +	if (actual_target == ATT_PARTITIONED_INDEX &&
> +			(allowed_targets&ATT_INDEX) &&
> +			!(allowed_targets&ATT_PARTITIONED_INDEX))
> +		/* Add a special errhint for this case, since "is not an index" message is unfriendly */
> +		ereport(ERROR,
> +				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +				 errmsg(msg, RelationGetRelationName(rel)),
> +				 // errhint("\"%s\" is a partitioned index", RelationGetRelationName(rel))));
> +				 errhint("operation is not supported on partitioned indexes")));
> +	else
> +		ereport(ERROR,
> +				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +				 errmsg(msg, RelationGetRelationName(rel))));
> +
>  }
>  
>  /*
> -- 
> 2.7.4
> 

-- 
Justin