rewrite ExecPartitionCheckEmitError
Just on cleanliness grounds, I propose to rewrite the function in $SUBJECT. I came across this while reviewing some already-committed patch for partition pruning, and it's been sitting in my laptop ever since. I think the current coding is too convoluted and hard to follow. The patch makes it much simpler (IMO). -- Álvaro Herrera >From 8c8ac713ac168b2678925e58cdfc83694a8fa5b9 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 14 Nov 2018 20:57:19 -0300 Subject: [PATCH] rewrite ExecPartitionCheckEmitError --- src/backend/executor/execMain.c | 36 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index d83d296d82..78c8ce4935 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1837,11 +1837,9 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) { - Relation rel = resultRelInfo->ri_RelationDesc; - Relation orig_rel = rel; - TupleDesc tupdesc = RelationGetDescr(rel); - char *val_desc; - Bitmapset *modifiedCols; + Oid root_relid; + TupleDesc tupdesc; + char *valdsc; Bitmapset *insertedCols; Bitmapset *updatedCols; @@ -1851,11 +1849,13 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, */ if (resultRelInfo->ri_PartitionRoot) { - TupleDesc old_tupdesc = RelationGetDescr(rel); + TupleDesc old_tupdesc; AttrNumber *map; - rel = resultRelInfo->ri_PartitionRoot; - tupdesc = RelationGetDescr(rel); + root_relid = RelationGetRelid(resultRelInfo->ri_PartitionRoot); + + old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc); + tupdesc = RelationGetDescr(resultRelInfo->ri_PartitionRoot); /* a reverse map */ map = convert_tuples_by_name_map_if_req(old_tupdesc, tupdesc, gettext_noop("could not convert row type")); @@ -1868,20 +1868,24 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, slot = execute_attr_map_slot(map, slot, MakeTupleTableSlot(tupdesc, &TTSOpsVirtual)); } + else + { + root_relid = RelationGetRelid(resultRelInfo->ri_RelationDesc); + tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc); + } insertedCols = GetInsertedColumns(resultRelInfo, estate); updatedCols = GetUpdatedColumns(resultRelInfo, estate); - modifiedCols = bms_union(insertedCols, updatedCols); - val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), - slot, - tupdesc, - modifiedCols, - 64); + valdsc = ExecBuildSlotValueDescription(root_relid, + slot, + tupdesc, + bms_union(insertedCols, updatedCols), + 64); ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), errmsg("new row for relation \"%s\" violates partition constraint", - RelationGetRelationName(orig_rel)), - val_desc ? errdetail("Failing row contains %s.", val_desc) : 0)); + RelationGetRelationName(resultRelInfo->ri_RelationDesc)), + valdsc ? errdetail("Failing row contains %s.", valdsc) : 0)); } /* -- 2.11.0
rewrite ExecPartitionCheckEmitError
Just on cleanliness grounds, I propose to rewrite the function in $SUBJECT. I came across this while reviewing some already-committed patch for partition pruning, and it's been sitting in my laptop ever since. I think the current coding is too convoluted and hard to follow. The patch makes it much simpler (IMO). -- Álvaro Herrera >From 8c8ac713ac168b2678925e58cdfc83694a8fa5b9 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 14 Nov 2018 20:57:19 -0300 Subject: [PATCH] rewrite ExecPartitionCheckEmitError --- src/backend/executor/execMain.c | 36 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index d83d296d82..78c8ce4935 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1837,11 +1837,9 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate) { - Relation rel = resultRelInfo->ri_RelationDesc; - Relation orig_rel = rel; - TupleDesc tupdesc = RelationGetDescr(rel); - char *val_desc; - Bitmapset *modifiedCols; + Oid root_relid; + TupleDesc tupdesc; + char *valdsc; Bitmapset *insertedCols; Bitmapset *updatedCols; @@ -1851,11 +1849,13 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, */ if (resultRelInfo->ri_PartitionRoot) { - TupleDesc old_tupdesc = RelationGetDescr(rel); + TupleDesc old_tupdesc; AttrNumber *map; - rel = resultRelInfo->ri_PartitionRoot; - tupdesc = RelationGetDescr(rel); + root_relid = RelationGetRelid(resultRelInfo->ri_PartitionRoot); + + old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc); + tupdesc = RelationGetDescr(resultRelInfo->ri_PartitionRoot); /* a reverse map */ map = convert_tuples_by_name_map_if_req(old_tupdesc, tupdesc, gettext_noop("could not convert row type")); @@ -1868,20 +1868,24 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, slot = execute_attr_map_slot(map, slot, MakeTupleTableSlot(tupdesc, &TTSOpsVirtual)); } + else + { + root_relid = RelationGetRelid(resultRelInfo->ri_RelationDesc); + tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc); + } insertedCols = GetInsertedColumns(resultRelInfo, estate); updatedCols = GetUpdatedColumns(resultRelInfo, estate); - modifiedCols = bms_union(insertedCols, updatedCols); - val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), - slot, - tupdesc, - modifiedCols, - 64); + valdsc = ExecBuildSlotValueDescription(root_relid, + slot, + tupdesc, + bms_union(insertedCols, updatedCols), + 64); ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), errmsg("new row for relation \"%s\" violates partition constraint", - RelationGetRelationName(orig_rel)), - val_desc ? errdetail("Failing row contains %s.", val_desc) : 0)); + RelationGetRelationName(resultRelInfo->ri_RelationDesc)), + valdsc ? errdetail("Failing row contains %s.", valdsc) : 0)); } /* -- 2.11.0
Re: rewrite ExecPartitionCheckEmitError
On Thu, Dec 6, 2018 at 5:22 PM Alvaro Herrera wrote: > Just on cleanliness grounds, I propose to rewrite the function in > $SUBJECT. I came across this while reviewing some already-committed > patch for partition pruning, and it's been sitting in my laptop ever > since. > > I think the current coding is too convoluted and hard to follow. The > patch makes it much simpler (IMO). I don't really think this is an improvement, but the only part I really dislike is s/val_desc/valdsc/. That is surely not an improvement in readability. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: rewrite ExecPartitionCheckEmitError
On 2018-Dec-07, Robert Haas wrote: > On Thu, Dec 6, 2018 at 5:22 PM Alvaro Herrera > wrote: > > Just on cleanliness grounds, I propose to rewrite the function in > > $SUBJECT. I came across this while reviewing some already-committed > > patch for partition pruning, and it's been sitting in my laptop ever > > since. > > > > I think the current coding is too convoluted and hard to follow. The > > patch makes it much simpler (IMO). > > I don't really think this is an improvement, but the only part I > really dislike is s/val_desc/valdsc/. That is surely not an > improvement in readability. Point taken; pushed with that change. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: rewrite ExecPartitionCheckEmitError
On Sat, 29 Dec 2018 at 06:52, Alvaro Herrera wrote: > Point taken; pushed with that change. Marking this as committed in the CF app. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: rewrite ExecPartitionCheckEmitError
On Thu, Dec 6, 2018 at 5:22 PM Alvaro Herrera wrote: > Just on cleanliness grounds, I propose to rewrite the function in > $SUBJECT. I came across this while reviewing some already-committed > patch for partition pruning, and it's been sitting in my laptop ever > since. > > I think the current coding is too convoluted and hard to follow. The > patch makes it much simpler (IMO). I don't really think this is an improvement, but the only part I really dislike is s/val_desc/valdsc/. That is surely not an improvement in readability. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: rewrite ExecPartitionCheckEmitError
On 2018-Dec-07, Robert Haas wrote: > On Thu, Dec 6, 2018 at 5:22 PM Alvaro Herrera > wrote: > > Just on cleanliness grounds, I propose to rewrite the function in > > $SUBJECT. I came across this while reviewing some already-committed > > patch for partition pruning, and it's been sitting in my laptop ever > > since. > > > > I think the current coding is too convoluted and hard to follow. The > > patch makes it much simpler (IMO). > > I don't really think this is an improvement, but the only part I > really dislike is s/val_desc/valdsc/. That is surely not an > improvement in readability. Point taken; pushed with that change. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: rewrite ExecPartitionCheckEmitError
On Sat, 29 Dec 2018 at 06:52, Alvaro Herrera wrote: > Point taken; pushed with that change. Marking this as committed in the CF app. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services