PartitionDispatch's partdesc field
I noticed today that in the PartitionDispatch structure, the partdesc
field is set but not used. So we could remove it. See attached
pd-partdesc-remove.patch. If we want to go this route, I suggest
doing a slightly more thorough removal and getting rid of the key
field as well. See attached pd-partdesc-and-key-remove.patch.
Another alternative, which I think might make more sense, is to make
use pd->key and pd->partdesc in preference to pd->reldesc->rd_partkey
and pd->reldesc->rd_partdesc. It seems to me that the idea of the
PartitionDispatch structure is that it gathers together all of the
information that we need for tuple routing, so it might make sense for
the tuple routing code ought to get the information from there rather
than referring back to the RelationDesc. See attached
pd-partdesc-use.patch.
Basically, the decision here is whether we want to avoid invoking
RelationGetPartitionKey and RelationGetPartitionDesc over and over
again, either for reasons of notational cleanliness (macro names are
long) or safety (absolutely no chance that the answer will change) or
efficiency (maybe those macros will someday do more than just a
pointer dereference?). If so, we should cache the data in the
PartitionDispatch object and then use it. If not, there seems to be
no reason to copy those pointers from the Relation to the
PartitionDispatch in the first place, since the latter has a pointer
to the former anyway.
Opinions?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
pd-partdesc-remove.patchapplication/octet-stream; name=pd-partdesc-remove.patchDownload
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 7a4665cc4e..174afad093 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -939,7 +939,6 @@ get_partition_dispatch_recurse(Relation rel, Relation parent,
pd->reldesc = rel;
pd->key = partkey;
pd->keystate = NIL;
- pd->partdesc = partdesc;
if (parent != NULL)
{
/*
diff --git a/src/include/executor/execPartition.h b/src/include/executor/execPartition.h
index 862bf65060..b67fef2914 100644
--- a/src/include/executor/execPartition.h
+++ b/src/include/executor/execPartition.h
@@ -25,7 +25,6 @@
* reldesc Relation descriptor of the table
* key Partition key information of the table
* keystate Execution state required for expressions in the partition key
- * partdesc Partition descriptor of the table
* tupslot A standalone TupleTableSlot initialized with this table's tuple
* descriptor
* tupmap TupleConversionMap to convert from the parent's rowtype to
@@ -41,7 +40,6 @@ typedef struct PartitionDispatchData
Relation reldesc;
PartitionKey key;
List *keystate; /* list of ExprState */
- PartitionDesc partdesc;
TupleTableSlot *tupslot;
TupleConversionMap *tupmap;
int *indexes;
pd-partdesc-and-key-remove.patchapplication/octet-stream; name=pd-partdesc-and-key-remove.patchDownload
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 7a4665cc4e..81fe6cdcf7 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -927,7 +927,6 @@ get_partition_dispatch_recurse(Relation rel, Relation parent,
{
TupleDesc tupdesc = RelationGetDescr(rel);
PartitionDesc partdesc = RelationGetPartitionDesc(rel);
- PartitionKey partkey = RelationGetPartitionKey(rel);
PartitionDispatch pd;
int i;
@@ -937,9 +936,7 @@ get_partition_dispatch_recurse(Relation rel, Relation parent,
pd = (PartitionDispatch) palloc(sizeof(PartitionDispatchData));
*pds = lappend(*pds, pd);
pd->reldesc = rel;
- pd->key = partkey;
pd->keystate = NIL;
- pd->partdesc = partdesc;
if (parent != NULL)
{
/*
@@ -1027,23 +1024,24 @@ FormPartitionKeyDatum(PartitionDispatch pd,
Datum *values,
bool *isnull)
{
+ PartitionKey key = RelationGetPartitionKey(pd->reldesc);
ListCell *partexpr_item;
int i;
- if (pd->key->partexprs != NIL && pd->keystate == NIL)
+ if (key->partexprs != NIL && pd->keystate == NIL)
{
/* Check caller has set up context correctly */
Assert(estate != NULL &&
GetPerTupleExprContext(estate)->ecxt_scantuple == slot);
/* First time through, set up expression evaluation state */
- pd->keystate = ExecPrepareExprList(pd->key->partexprs, estate);
+ pd->keystate = ExecPrepareExprList(key->partexprs, estate);
}
partexpr_item = list_head(pd->keystate);
- for (i = 0; i < pd->key->partnatts; i++)
+ for (i = 0; i < key->partnatts; i++)
{
- AttrNumber keycol = pd->key->partattrs[i];
+ AttrNumber keycol = key->partattrs[i];
Datum datum;
bool isNull;
diff --git a/src/include/executor/execPartition.h b/src/include/executor/execPartition.h
index 862bf65060..6f6a8d56a7 100644
--- a/src/include/executor/execPartition.h
+++ b/src/include/executor/execPartition.h
@@ -23,25 +23,21 @@
* hierarchy required to route a tuple to one of its partitions
*
* reldesc Relation descriptor of the table
- * key Partition key information of the table
* keystate Execution state required for expressions in the partition key
- * partdesc Partition descriptor of the table
* tupslot A standalone TupleTableSlot initialized with this table's tuple
* descriptor
* tupmap TupleConversionMap to convert from the parent's rowtype to
* this table's rowtype (when extracting the partition key of a
* tuple just before routing it through this table)
- * indexes Array with partdesc->nparts members (for details on what
- * individual members represent, see how they are set in
- * get_partition_dispatch_recurse())
+ * indexes Array with as many members as the rel's PartitionDesc (for
+ * details on what individual members represent, see how they
+ * are set in get_partition_dispatch_recurse())
*-----------------------
*/
typedef struct PartitionDispatchData
{
Relation reldesc;
- PartitionKey key;
List *keystate; /* list of ExprState */
- PartitionDesc partdesc;
TupleTableSlot *tupslot;
TupleConversionMap *tupmap;
int *indexes;
pd-partdesc-use.patchapplication/octet-stream; name=pd-partdesc-use.patchDownload
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 7a4665cc4e..cd0ec08461 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -41,7 +41,7 @@ static void FormPartitionKeyDatum(PartitionDispatch pd,
EState *estate,
Datum *values,
bool *isnull);
-static int get_partition_for_tuple(Relation relation, Datum *values,
+static int get_partition_for_tuple(PartitionDispatch pd, Datum *values,
bool *isnull);
static char *ExecBuildSlotPartitionKeyDescription(Relation rel,
Datum *values,
@@ -208,13 +208,11 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
parent = pd[0];
while (true)
{
- PartitionDesc partdesc;
TupleTableSlot *myslot = parent->tupslot;
TupleConversionMap *map = parent->tupmap;
int cur_index = -1;
rel = parent->reldesc;
- partdesc = RelationGetPartitionDesc(rel);
/*
* Convert the tuple to this parent's layout so that we can do certain
@@ -245,13 +243,13 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd,
* Nothing for get_partition_for_tuple() to do if there are no
* partitions to begin with.
*/
- if (partdesc->nparts == 0)
+ if (parent->partdesc->nparts == 0)
{
result = -1;
break;
}
- cur_index = get_partition_for_tuple(rel, values, isnull);
+ cur_index = get_partition_for_tuple(parent, values, isnull);
/*
* cur_index < 0 means we failed to find a partition of this parent.
@@ -1079,12 +1077,12 @@ FormPartitionKeyDatum(PartitionDispatch pd,
* found or -1 if none found.
*/
static int
-get_partition_for_tuple(Relation relation, Datum *values, bool *isnull)
+get_partition_for_tuple(PartitionDispatch pd, Datum *values, bool *isnull)
{
int bound_offset;
int part_index = -1;
- PartitionKey key = RelationGetPartitionKey(relation);
- PartitionDesc partdesc = RelationGetPartitionDesc(relation);
+ PartitionKey key = pd->key;
+ PartitionDesc partdesc = pd->partdesc;
PartitionBoundInfo boundinfo = partdesc->boundinfo;
/* Route as appropriate based on partitioning strategy. */
On 2018/07/26 5:23, Robert Haas wrote:
I noticed today that in the PartitionDispatch structure, the partdesc
field is set but not used. So we could remove it. See attached
pd-partdesc-remove.patch. If we want to go this route, I suggest
doing a slightly more thorough removal and getting rid of the key
field as well. See attached pd-partdesc-and-key-remove.patch.Another alternative, which I think might make more sense, is to make
use pd->key and pd->partdesc in preference to pd->reldesc->rd_partkey
and pd->reldesc->rd_partdesc. It seems to me that the idea of the
PartitionDispatch structure is that it gathers together all of the
information that we need for tuple routing, so it might make sense for
the tuple routing code ought to get the information from there rather
than referring back to the RelationDesc. See attached
pd-partdesc-use.patch.
+1 to pd-partdesc-use.patch.
Basically, the decision here is whether we want to avoid invoking
RelationGetPartitionKey and RelationGetPartitionDesc over and over
again, either for reasons of notational cleanliness (macro names are
long) or safety (absolutely no chance that the answer will change) or
efficiency (maybe those macros will someday do more than just a
pointer dereference?). If so, we should cache the data in the
PartitionDispatch object and then use it.
I agree.
Thanks,
Amit
On Wed, Jul 25, 2018 at 10:42 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
Another alternative, which I think might make more sense, is to make
use pd->key and pd->partdesc in preference to pd->reldesc->rd_partkey
and pd->reldesc->rd_partdesc. It seems to me that the idea of the
PartitionDispatch structure is that it gathers together all of the
information that we need for tuple routing, so it might make sense for
the tuple routing code ought to get the information from there rather
than referring back to the RelationDesc. See attached
pd-partdesc-use.patch.+1 to pd-partdesc-use.patch.
OK, that makes 2 votes for that alternative and 0 for everything else
combined, so I've committed that version.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company