Hello,

At Mon, 25 Jul 2016 18:21:27 +0900, Amit Langote 
<langote_amit...@lab.ntt.co.jp> wrote in 
<96fb8bca-57f5-e5a8-9630-79d4fc5b2...@lab.ntt.co.jp>
> 
> Hello,
> 
> On 2016/07/25 17:18, Kyotaro HORIGUCHI wrote:
> > 
> >  - Remove ccvalid condition from equalTupleDescs() to reduce
> >    unnecessary cache invalidation or tuple rebuilding.
> 
> The following commit introduced the ccvalid check:
> 
> """
> commit c31305de5f5a4880b0ba2f5983025ef0210a3b2a
> Author: Noah Misch <n...@leadboat.com>
> Date:   Sun Mar 23 02:13:43 2014 -0400
> 
> Address ccvalid/ccnoinherit in TupleDesc support functions.
> 
> equalTupleDescs() neglected both of these ConstrCheck fields, and
> CreateTupleDescCopyConstr() neglected ccnoinherit.  At this time, the
> only known behavior defect resulting from these omissions is constraint
> exclusion disregarding a CHECK constraint validated by an ALTER TABLE
> VALIDATE CONSTRAINT statement issued earlier in the same transaction.
> Back-patch to 9.2, where these fields were introduced.
> """

Wow.. Missed the obvious thing. Certainly relation cache must be
invalidated by a change of ccvalidated as the commit message.

> So, apparently RelationClearRelation() does intend to discard a cached
> TupleDesc if ccvalid changed in a transaction.  Whereas,
> acquire_inherited_sample_rows() does not seem to depend on ccvalid being
> equal or not (or any member of TupleConstr for that matter).  So maybe,
> instead of simply discarding the check (which does serve a purpose), we
> could make equalTupleDescs() parameterized on whether we want TupleConstr
> equality check to be performed or not.  RelationClearRelation() can ask
> for the check to be performed by passing true for the parameter whereas
> acquire_inherited_sample_rows() and other callers can pass false.  Perhaps
> something like the attached.

Hmm. It should resolve the problem. But the two comparisons seem
to be separate things. Constraints is not a part of tuple
description. relcache seems to be making misusage of the equality
of tuple descriptors.

So, how about splitting the original equalTupleDescs into
equalTupleDescs and equalTupleConstraints ?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 5a1d570ac327702fea19ed18198efeb02632a2f3 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Tue, 26 Jul 2016 11:02:48 +0900
Subject: [PATCH] Split equalTupleDescs into two functions.

equalTupleDescs intends to the two given tuple descriptors are
compatible on generating a tuple. Comaprison on constraints are
required to invalidate cache but it is a separate matter. So split it
into two new functions equalTupleDescs and equalTupleConstraints.
---
 src/backend/access/common/tupdesc.c | 14 ++++++++++++++
 src/include/access/tupdesc.h        |  1 +
 2 files changed, 15 insertions(+)

diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index b56d0e3..1e42c87 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -411,6 +411,19 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 		/* attacl, attoptions and attfdwoptions are not even present... */
 	}
 
+	return true;
+}
+
+/*
+ * Compare constraints between two TupleDesc structures
+ */
+bool
+equalTupleConstraints(TupleDesc tupdesc1, TupleDesc tupdesc2)
+{
+	int			i,
+				j,
+				n;
+
 	if (tupdesc1->constr != NULL)
 	{
 		TupleConstr *constr1 = tupdesc1->constr;
@@ -470,6 +483,7 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 	}
 	else if (tupdesc2->constr != NULL)
 		return false;
+
 	return true;
 }
 
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index de18f74..58fa466 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -111,6 +111,7 @@ extern void DecrTupleDescRefCount(TupleDesc tupdesc);
 	} while (0)
 
 extern bool equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
+extern bool equalTupleConstraints(TupleDesc tupdesc1, TupleDesc tupdesc2);
 
 extern void TupleDescInitEntry(TupleDesc desc,
 				   AttrNumber attributeNumber,
-- 
1.8.3.1

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to