On 2017/01/25 2:56, Robert Haas wrote:
> On Thu, Jan 19, 2017 at 9:58 PM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>>> But I wonder why we don't instead just change this function to
>>> consider tdhasoid rather than tdtypeid. I mean, if the only point of
>>> comparing the type OIDs is to find out whether the table-has-OIDs
>>> setting matches, we could instead test that directly and avoid needing
>>> to pass an extra argument. I wonder if there's some other reason this
>>> code is there which is not documented in the comment...
>>
>> With the following patch, regression tests run fine:
>>
>> if (indesc->natts == outdesc->natts &&
>> - indesc->tdtypeid == outdesc->tdtypeid)
>> + indesc->tdhasoid != outdesc->tdhasoid)
>> {
>>
>> If checking tdtypeid (instead of tdhasoid directly) has some other
>> consideration, I'd would have seen at least some tests broken by this
>> change. So, if we are to go with this, I too prefer it over my previous
>> proposal to add an argument to convert_tuples_by_name(). Attached 0003
>> implements the above approach.
>
> I think this is not quite right. First, the patch compares the
> tdhasoid status with != rather than ==, which would have the effect of
> saying that we can skip conversion of the has-OID statuses do NOT
> match. That can't be right.
You're right.
> Second, I believe that the comments
> imply that conversion should be done if *either* tuple has OIDs. I
> believe that's because whoever wrote this comment thought that we
> needed to replace the OID if the tuple already had one, which is what
> do_convert_tuple would do. I'm not sure whether that's really
> necessary, but we're less likely to break anything if we preserve the
> existing behavior, and I don't think we lose much from doing so
> because few user tables will have OIDs. So I would change this test
> to if (indesc->natts == outdesc->natts && !indesc->tdhasoid &&
> !outdesc->tdhasoid), and I'd revise the one in
> convert_tuples_by_position() to match. Then I think it's much clearer
> that we're just optimizing what's there already, not changing the
> behavior.
Agreed. Updated patch attached.
Thanks,
Amit
>From c1fa4b9f04f328b8df54ef487ee9d46f5978e0de Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 26 Dec 2016 17:44:14 +0900
Subject: [PATCH] Avoid tuple coversion in common partitioning cases
Currently, the tuple conversion is performed after a tuple is routed,
even if the attributes of a target leaf partition map one-to-one with
those of the root table, which is wasteful. Avoid that by making
convert_tuples_by_name() return a NULL map for such cases.
Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
src/backend/access/common/tupconvert.c | 18 ++++++++++--------
src/backend/catalog/partition.c | 3 +--
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/common/tupconvert.c b/src/backend/access/common/tupconvert.c
index b17ceafa6e..a4012525d8 100644
--- a/src/backend/access/common/tupconvert.c
+++ b/src/backend/access/common/tupconvert.c
@@ -138,12 +138,13 @@ convert_tuples_by_position(TupleDesc indesc,
nincols, noutcols)));
/*
- * Check to see if the map is one-to-one and the tuple types are the same.
- * (We check the latter because if they're not, we want to do conversion
- * to inject the right OID into the tuple datum.)
+ * Check to see if the map is one-to-one, in which case we need not do
+ * the tuple conversion. That's not enough though if either source or
+ * destination (tuples) contains OIDs; we'd need conversion in that case
+ * to inject the right OID into the tuple datum.
*/
if (indesc->natts == outdesc->natts &&
- indesc->tdtypeid == outdesc->tdtypeid)
+ !indesc->tdhasoid && !outdesc->tdhasoid)
{
for (i = 0; i < n; i++)
{
@@ -214,12 +215,13 @@ convert_tuples_by_name(TupleDesc indesc,
attrMap = convert_tuples_by_name_map(indesc, outdesc, msg);
/*
- * Check to see if the map is one-to-one and the tuple types are the same.
- * (We check the latter because if they're not, we want to do conversion
- * to inject the right OID into the tuple datum.)
+ * Check to see if the map is one-to-one, in which case we need not do
+ * the tuple conversion. That's not enough though if either source or
+ * destination (tuples) contains OIDs; we'd need conversion in that case
+ * to inject the right OID into the tuple datum.
*/
if (indesc->natts == outdesc->natts &&
- indesc->tdtypeid == outdesc->tdtypeid)
+ !indesc->tdhasoid && !outdesc->tdhasoid)
{
same = true;
for (i = 0; i < n; i++)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 7be60529c5..4bcef58763 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1700,12 +1700,11 @@ get_partition_for_tuple(PartitionDispatch *pd,
return -1;
}
- if (myslot != NULL)
+ if (myslot != NULL && map != NULL)
{
HeapTuple tuple = ExecFetchSlotTuple(slot);
ExecClearTuple(myslot);
- Assert(map != NULL);
tuple = do_convert_tuple(tuple, map);
ExecStoreTuple(tuple, myslot, InvalidBuffer, true);
slot = myslot;
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
