> 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, LOCKMODE lockmode) UnlockRelationId(&relid, lockmode); } +/* + * Check Relation after opening. Internal procedure used by heap_open and + * heap_openrv_* procedures. + */ +inline 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); } /* ---------------- @@ -1342,7 +1322,7 @@ heap_openrv(const RangeVar *relation, LOCKMODE lockmode) * relation-not-found. * ---------------- */ -Relation +inline Relation heap_openrv_extended(const RangeVar *relation, LOCKMODE lockmode, bool missing_ok) { @@ -1351,18 +1331,7 @@ heap_openrv_extended(const RangeVar *relation, LOCKMODE lockmode, r = relation_openrv_extended(relation, lockmode, missing_ok); if (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_check_relation(r); return r; }
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers