Re: [HACKERS] Small patch: fix code duplication in heapam.c

2016-03-25 Thread Aleksander Alekseev 
> I think this is a waste of time.  These functions are already very
> short; making them shorter will not significantly improve readability.
> It'll just force people who think they know what that code does to
> look at it again to see if it still does the same thing.
> 
> Let's spend our time arguing about changes that matter.  There are an
> infinite number of things like this that you could tinker with, but
> most of them are not worth tinkering with.

I must respectfully disagree. Granted, this is not a big issue and
we don't have to fix it right now. Probably next commit fest would be a
better time.

But this is not a huge patch that changing everything in unpredictable
way and requires a lot of hard thinking. We also have code review,
regression tests, alpha and beta tests to be reasonably sure that such
change doesn't break anything. (If not perhaps we should improve this
situation by introducing new ways of modular and property-based
testing, which I believe would be extremely useful say in case of
indexes, but this is a different story).

I don't believe we can afford to keep such a confusing code using
provided arguments as an excuse not to fixing it. ("OK, there are two
procedures that work differently... lets see... or not? well, thats
odd... lets make :vsplit and compare them line by line... damn, I spend
all that time to figure out that they are the same!") Otherwise such
"broken windows" will accumulate until it become a _real_ problem.
As we know from experience, to that time it's usually much harder to
fix anything than it is now.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/


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

Re: [HACKERS] Small patch: fix code duplication in heapam.c

2016-03-25 Thread Robert Haas 
On Thu, Mar 24, 2016 at 11:35 AM, Aleksander Alekseev
 wrote:
> In the same time I'm deeply convinced that this patch will make code
> more readable at least because it makes code much shorter:

I think this is a waste of time.  These functions are already very
short; making them shorter will not significantly improve readability.
It'll just force people who think they know what that code does to
look at it again to see if it still does the same thing.

Let's spend our time arguing about changes that matter.  There are an
infinite number of things like this that you could tinker with, but
most of them are not worth tinkering with.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Small patch: fix code duplication in heapam.c

2016-03-24 Thread Aleksander Alekseev 
> All that code is hotspot stuff, and turning it into a pile of nested
> procedures doesn't seem like it improves either performance or
> readability.

Your concern regarding performance is understandable. But I should note
that any standard compiler supports inlining these days (probably
this statement is true for at least a decade now). Here is assembly code
of patched version of heap_open produced by GCC 4.8.4 with -O2 flag:

(lldb) disassemble 
postgres`heap_open:
0x497db0 <+0>:   pushq  %rbx
0x497db1 <+1>:   callq  0x497af0  ; relation_open(...)
0x497db6 <+6>:   movq   %rax, %rbx
->  0x497db9 <+9>:   movq   0x30(%rax), %rax
0x497dbd <+13>:  movzbl 0x6f(%rax), %eax
0x497dc1 <+17>:  cmpb   $0x69, %al; 'i', RELKIND_INDEX
0x497dc3 <+19>:  je 0x497dce
0x497dc5 <+21>:  cmpb   $0x63, %al; 'c', COMPOSITE_TYPE
0x497dc7 <+23>:  je 0x497dd7
0x497dc9 <+25>:  movq   %rbx, %rax
0x497dcc <+28>:  popq   %rbx
0x497dcd <+29>:  retq   

As you see heap_open_check_relation procedure was successfully inlined.
Just to be sure that less smart compilers will more likely apply this
optimization I updated patch with 'inline' hints (see attachment).

And even if compiler decide not to apply inlining in this case there is
much more to consider than presence or absence of one 'call' assembly
instruction. For instance compiler may believe that on this concrete
architecture it will be more beneficial to make code shorter so it
would fit to CPU cache better.

Anyway I don't believe that imaginary benchmarks are worth trusting. I
personally don't have much faith in non-imaginary benchmarks either but
it's a different story.

In the same time I'm deeply convinced that this patch will make code
more readable at least because it makes code much shorter:

 src/backend/access/heap/heapam.c | 109 +++---
 1 file changed, 39 insertions(+), 70 deletions(-)

Thus we can see more code on the screen. Besides since there is no code
duplication there is less change that somebody someday will change say
heap_openrv without updating heap_openrv_extended accordingly. 

-- 
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 34ba385..c55e6a7 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1188,13 +1188,17 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
 }
 
 /* 
- *		relation_openrv - open any relation specified by a RangeVar
+ *		relation_openrv_extended - open any relation specified by a RangeVar
  *
- *		Same as relation_open, but the relation is specified by a RangeVar.
+ *		Same as relation_openrv, but with an additional missing_ok argument
+ *		allowing a NULL return rather than an error if the relation is not
+ *		found.  (Note that some other causes, such as permissions problems,
+ *		will still result in an ereport.)
  * 
  */
-Relation
-relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
+inline Relation
+relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
+		 bool missing_ok)
 {
 	Oid			relOid;
 
@@ -1213,43 +1217,26 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 		AcceptInvalidationMessages();
 
 	/* Look up and lock the appropriate relation using namespace search */
-	relOid = RangeVarGetRelid(relation, lockmode, false);
+	relOid = RangeVarGetRelid(relation, lockmode, missing_ok);
+
+	/* Return NULL on not-found */
+	if (!OidIsValid(relOid))
+		return NULL;
 
 	/* Let relation_open do the rest */
 	return relation_open(relOid, NoLock);
 }
 
 /* 
- *		relation_openrv_extended - open any relation specified by a RangeVar
+ *		relation_openrv - open any relation specified by a RangeVar
  *
- *		Same as relation_openrv, but with an additional missing_ok argument
- *		allowing a NULL return rather than an error if the relation is not
- *		found.  (Note that some other causes, such as permissions problems,
- *		will still result in an ereport.)
+ *		Same as relation_open, but the relation is specified by a RangeVar.
  * 
  */
 Relation
-relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
-		 bool missing_ok)
+relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 {
-	Oid			relOid;
-
-	/*
-	 * Check for shared-cache-inval messages before trying to open the
-	 * relation.  See comments in relation_openrv().
-	 */
-	if (lockmode != NoLock)
-		AcceptInvalidationMessages();
-
-	/* Look up and lock the appropriate relation using namespace search */
-	relOid = RangeVarGetRelid(relation, lockmode, missing_ok);
-
-	/* Return NULL on not-found */
-	if (!OidIsValid(relOid))
-		return NULL;
-
-	/* Let relation_open do the rest */
-	return relation_open(relOid, NoLock);
+	return relation_openrv_extended(relation, lockmode, false);
 }
 
 /* 
@@ -1275,6 +1262,24 @@ relation_close(Relation relation,

Re: [HACKERS] Small patch: fix code duplication in heapam.c

2016-03-24 Thread Tom Lane 
Aleksander Alekseev  writes:
> I discovered that there is a lot of code duplication in heapam.c.
> In particular relation_openrv and relation_openrv_extended procedures
> and also heap_openrv and heap_openrv_extended procedures are almost the
> same. Here is a patch that fixes this.

As with that other patch to refactor palloc-related stuff, I'm not
convinced that this is an improvement.  All that code is hotspot stuff,
and turning it into a pile of nested procedures doesn't seem like it
improves either performance or readability.

regards, tom lane


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

[HACKERS] Small patch: fix code duplication in heapam.c

2016-03-24 Thread Aleksander Alekseev 
Hello

I discovered that there is a lot of code duplication in heapam.c.
In particular relation_openrv and relation_openrv_extended procedures
and also heap_openrv and heap_openrv_extended procedures are almost the
same. Here is a patch that fixes this.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 34ba385..e9db6ad 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1188,13 +1188,17 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
 }
 
 /* 
- *		relation_openrv - open any relation specified by a RangeVar
+ *		relation_openrv_extended - open any relation specified by a RangeVar
  *
- *		Same as relation_open, but the relation is specified by a RangeVar.
+ *		Same as relation_openrv, but with an additional missing_ok argument
+ *		allowing a NULL return rather than an error if the relation is not
+ *		found.  (Note that some other causes, such as permissions problems,
+ *		will still result in an ereport.)
  * 
  */
 Relation
-relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
+relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
+		 bool missing_ok)
 {
 	Oid			relOid;
 
@@ -1213,43 +1217,26 @@ relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 		AcceptInvalidationMessages();
 
 	/* Look up and lock the appropriate relation using namespace search */
-	relOid = RangeVarGetRelid(relation, lockmode, false);
+	relOid = RangeVarGetRelid(relation, lockmode, missing_ok);
+
+	/* Return NULL on not-found */
+	if (!OidIsValid(relOid))
+		return NULL;
 
 	/* Let relation_open do the rest */
 	return relation_open(relOid, NoLock);
 }
 
 /* 
- *		relation_openrv_extended - open any relation specified by a RangeVar
+ *		relation_openrv - open any relation specified by a RangeVar
  *
- *		Same as relation_openrv, but with an additional missing_ok argument
- *		allowing a NULL return rather than an error if the relation is not
- *		found.  (Note that some other causes, such as permissions problems,
- *		will still result in an ereport.)
+ *		Same as relation_open, but the relation is specified by a RangeVar.
  * 
  */
 Relation
-relation_openrv_extended(const RangeVar *relation, LOCKMODE lockmode,
-		 bool missing_ok)
+relation_openrv(const RangeVar *relation, LOCKMODE lockmode)
 {
-	Oid			relOid;
-
-	/*
-	 * Check for shared-cache-inval messages before trying to open the
-	 * relation.  See comments in relation_openrv().
-	 */
-	if (lockmode != NoLock)
-		AcceptInvalidationMessages();
-
-	/* Look up and lock the appropriate relation using namespace search */
-	relOid = RangeVarGetRelid(relation, lockmode, missing_ok);
-
-	/* Return NULL on not-found */
-	if (!OidIsValid(relOid))
-		return NULL;
-
-	/* Let relation_open do the rest */
-	return relation_open(relOid, NoLock);
+	return relation_openrv_extended(relation, lockmode, false);
 }
 
 /* 
@@ -1275,6 +1262,24 @@ relation_close(Relation relation, LOCKMODE lockmode)
 		UnlockRelationId(, lockmode);
 }
 
+/*
+ * Check Relation after opening. Internal procedure used by heap_open and
+ * heap_openrv_* procedures.
+ */
+static void
+heap_open_check_relation(Relation r)
+{
+	if (r->rd_rel->relkind == RELKIND_INDEX)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is an index",
+		RelationGetRelationName(r;
+	else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is a composite type",
+		RelationGetRelationName(r;
+}
 
 /* 
  *		heap_open - open a heap relation by relation OID
@@ -1291,17 +1296,7 @@ heap_open(Oid relationId, LOCKMODE lockmode)
 	Relation	r;
 
 	r = relation_open(relationId, lockmode);
-
-	if (r->rd_rel->relkind == RELKIND_INDEX)
-		ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
-		RelationGetRelationName(r;
-	else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
-		ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
-		RelationGetRelationName(r;
+	heap_open_check_relation(r);
 
 	return r;
 }
@@ -1316,22 +1311,7 @@ heap_open(Oid relationId, LOCKMODE lockmode)
 Relation
 heap_openrv(const RangeVar *relation, LOCKMODE lockmode)
 {
-	Relation	r;
-
-	r = relation_openrv(relation, lockmode);
-
-	if (r->rd_rel->relkind == RELKIND_INDEX)
-		ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is an index",
-		RelationGetRelationName(r;
-	else if (r->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
-		ereport(ERROR,
-(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is a composite type",
-		RelationGetRelationName(r;
-
-	return r;
+	return heap_openrv_extended(relation, lockmode, false);
 }
 
 /*