Skip to content

Commit 6b2c4e5

Browse files
tglsfdctglsfdc
committed
Improve error cursor positions for problems with partition bounds.
We failed to pass down the query string to check_new_partition_bound, so that its attempts to provide error cursor positions were for naught; one must have the query string to get parser_errposition to do anything. Adjust its API to require a ParseState to be passed down. Also, improve the logic inside check_new_partition_bound so that the cursor points at the partition bound for the specific column causing the issue, when one can be identified. That part is also for naught if we can't determine the query position of the column with the problem. Improve transformPartitionBoundValue so that it makes sure that const-simplified partition expressions will be properly labeled with positions. In passing, skip calling evaluate_expr if the value is already a Const, which is surely the most common case. Alexandra Wang, Ashwin Agrawal, Amit Langote; reviewed by Ashutosh Bapat Discussion: https://postgr.es/m/CACiyaSopZoqssfMzgHk6fAkp01cL6vnqBdmTw2C5_KJaFR_aMg@mail.gmail.com Discussion: https://postgr.es/m/CAJV4CdrZ5mKuaEsRSbLf2URQ3h6iMtKD=hik8MaF5WwdmC9uZw@mail.gmail.com
1 parent 3ea7e95 commit 6b2c4e5

File tree

6 files changed

+160
-58
lines changed

6 files changed

+160
-58
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,8 @@ static void ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partPa
543543
static void CreateInheritance(Relation child_rel, Relation parent_rel);
544544
static void RemoveInheritance(Relation child_rel, Relation parent_rel);
545545
static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel,
546-
PartitionCmd *cmd);
546+
PartitionCmd *cmd,
547+
AlterTableUtilityContext *context);
547548
static void AttachPartitionEnsureIndexes(Relation rel, Relation attachrel);
548549
static void QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
549550
List *partConstraint,
@@ -1007,7 +1008,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
10071008
* Check first that the new partition's bound is valid and does not
10081009
* overlap with any of existing partitions of the parent.
10091010
*/
1010-
check_new_partition_bound(relname, parent, bound);
1011+
check_new_partition_bound(relname, parent, bound, pstate);
10111012

10121013
/*
10131014
* If the default partition exists, its partition constraints will
@@ -4718,7 +4719,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
47184719
cur_pass, context);
47194720
Assert(cmd != NULL);
47204721
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
4721-
ATExecAttachPartition(wqueue, rel, (PartitionCmd *) cmd->def);
4722+
ATExecAttachPartition(wqueue, rel, (PartitionCmd *) cmd->def,
4723+
context);
47224724
else
47234725
ATExecAttachPartitionIdx(wqueue, rel,
47244726
((PartitionCmd *) cmd->def)->name);
@@ -16280,7 +16282,8 @@ QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
1628016282
* Return the address of the newly attached partition.
1628116283
*/
1628216284
static ObjectAddress
16283-
ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
16285+
ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
16286+
AlterTableUtilityContext *context)
1628416287
{
1628516288
Relation attachrel,
1628616289
catalog;
@@ -16295,6 +16298,9 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
1629516298
const char *trigger_name;
1629616299
Oid defaultPartOid;
1629716300
List *partBoundConstraint;
16301+
ParseState *pstate = make_parsestate(NULL);
16302+
16303+
pstate->p_sourcetext = context->queryString;
1629816304

1629916305
/*
1630016306
* We must lock the default partition if one exists, because attaching a
@@ -16460,7 +16466,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
1646016466
* error.
1646116467
*/
1646216468
check_new_partition_bound(RelationGetRelationName(attachrel), rel,
16463-
cmd->bound);
16469+
cmd->bound, pstate);
1646416470

1646516471
/* OK to create inheritance. Rest of the checks performed there */
1646616472
CreateInheritance(attachrel, rel);

‎src/backend/parser/parse_utilcmd.c

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4164,7 +4164,7 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
41644164
}
41654165

41664166
/*
4167-
* Transform one constant in a partition bound spec
4167+
* Transform one entry in a partition bound spec, producing a constant.
41684168
*/
41694169
static Const *
41704170
transformPartitionBoundValue(ParseState *pstate, Node *val,
@@ -4176,6 +4176,13 @@ transformPartitionBoundValue(ParseState *pstate, Node *val,
41764176
/* Transform raw parsetree */
41774177
value = transformExpr(pstate, val, EXPR_KIND_PARTITION_BOUND);
41784178

4179+
/*
4180+
* transformExpr() should have already rejected column references,
4181+
* subqueries, aggregates, window functions, and SRFs, based on the
4182+
* EXPR_KIND_ of a partition bound expression.
4183+
*/
4184+
Assert(!contain_var_clause(value));
4185+
41794186
/*
41804187
* Check that the input expression's collation is compatible with one
41814188
* specified for the parent's partition key (partcollation). Don't throw
@@ -4220,7 +4227,11 @@ transformPartitionBoundValue(ParseState *pstate, Node *val,
42204227
parser_errposition(pstate, exprLocation(value))));
42214228
}
42224229

4223-
/* Coerce to correct type */
4230+
/*
4231+
* Coerce to the correct type. This might cause an explicit coercion step
4232+
* to be added on top of the expression, which must be evaluated before
4233+
* returning the result to the caller.
4234+
*/
42244235
value = coerce_to_target_type(pstate,
42254236
value, exprType(value),
42264237
colType,
@@ -4236,25 +4247,35 @@ transformPartitionBoundValue(ParseState *pstate, Node *val,
42364247
format_type_be(colType), colName),
42374248
parser_errposition(pstate, exprLocation(val))));
42384249

4239-
/* Simplify the expression, in case we had a coercion */
4240-
if (!IsA(value, Const))
4241-
value = (Node *) expression_planner((Expr *) value);
4242-
42434250
/*
4244-
* transformExpr() should have already rejected column references,
4245-
* subqueries, aggregates, window functions, and SRFs, based on the
4246-
* EXPR_KIND_ for a default expression.
4251+
* Evaluate the expression, if needed, assigning the partition key's data
4252+
* type and collation to the resulting Const node.
42474253
*/
4248-
Assert(!contain_var_clause(value));
4254+
if (!IsA(value, Const))
4255+
{
4256+
value = (Node *) expression_planner((Expr *) value);
4257+
value = (Node *) evaluate_expr((Expr *) value, colType, colTypmod,
4258+
partCollation);
4259+
if (!IsA(value, Const))
4260+
elog(ERROR, "could not evaluate partition bound expression");
4261+
}
4262+
else
4263+
{
4264+
/*
4265+
* If the expression is already a Const, as is often the case, we can
4266+
* skip the rather expensive steps above. But we still have to insert
4267+
* the right collation, since coerce_to_target_type doesn't handle
4268+
* that.
4269+
*/
4270+
((Const *) value)->constcollid = partCollation;
4271+
}
42494272

42504273
/*
4251-
* Evaluate the expression, assigning the partition key's collation to the
4252-
* resulting Const expression.
4274+
* Attach original expression's parse location to the Const, so that
4275+
* that's what will be reported for any later errors related to this
4276+
* partition bound.
42534277
*/
4254-
value = (Node *) evaluate_expr((Expr *) value, colType, colTypmod,
4255-
partCollation);
4256-
if (!IsA(value, Const))
4257-
elog(ERROR, "could not evaluate partition bound expression");
4278+
((Const *) value)->location = exprLocation(val);
42584279

42594280
return (Const *) value;
42604281
}

0 commit comments

Comments
 (0)