[PATCH] Change simple_heap_insert() to a macro

Started by Andrey Klychkovover 7 years ago5 messages

aaklychkov@mail.ru

over 7 years ago

1 attachment(s)

Hi, Hackers
Studying another question I noticed a small point for optimization.
In the src/backend/access/heap/heapam.c we have the function:
- * simple_heap_insert - insert a tuple
- *
- * Currently, this routine differs from heap_insert only in supplying
- * a default command ID and not allowing access to the speedup options.
- *
- * This should be used rather than using heap_insert directly in most places
- * where we are modifying system catalogs.
- */
-Oid
-simple_heap_insert(Relation relation, HeapTuple tup)
-{
- return heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL);
-}
I changed it to a macro. See the attached patch.
I will be grateful if someone look at this.
Thank you! --
Regards,
Andrey Klychkov

Attachments:

v1-00-simple_heap_insert-to-macro.patchapplication/x-patch; name="=?UTF-8?B?djEtMDAtc2ltcGxlX2hlYXBfaW5zZXJ0LXRvLW1hY3JvLnBhdGNo?="Download

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index fb63471..ce8cdb9 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2410,6 +2410,11 @@ ReleaseBulkInsertStatePin(BulkInsertState bistate)
 /*
  *	heap_insert		- insert tuple into a heap
  *
+ *  NB: simple_heap_insert macro should be used rather than using heap_insert
+ *  directly in most places where we are modifying system catalogs.
+ *  Currently, this routine differs from heap_insert only in supplying
+ *  a default command ID and not allowing access to the speedup options.
+ *
  * The new tuple is stamped with current transaction ID and the specified
  * command ID.
  *
@@ -2987,21 +2992,6 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
 }
 
 /*
- *	simple_heap_insert - insert a tuple
- *
- * Currently, this routine differs from heap_insert only in supplying
- * a default command ID and not allowing access to the speedup options.
- *
- * This should be used rather than using heap_insert directly in most places
- * where we are modifying system catalogs.
- */
-Oid
-simple_heap_insert(Relation relation, HeapTuple tup)
-{
-	return heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL);
-}
-
-/*
  * Given infomask/infomask2, compute the bits that must be saved in the
  * "infobits" field of xl_heap_delete, xl_heap_update, xl_heap_lock,
  * xl_heap_lock_updated WAL records.
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 40e153f..0941778 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -16,6 +16,7 @@
 
 #include "access/sdir.h"
 #include "access/skey.h"
+#include "access/xact.h"
 #include "nodes/lockoptions.h"
 #include "nodes/primnodes.h"
 #include "storage/bufpage.h"
@@ -107,6 +108,18 @@ typedef struct ParallelHeapScanDescData *ParallelHeapScanDesc;
  */
 #define HeapScanIsValid(scan) PointerIsValid(scan)
 
+/*
+ *	simple_heap_insert - insert a tuple
+ *
+ * Currently, this routine differs from heap_insert only in supplying
+ * a default command ID and not allowing access to the speedup options.
+ *
+ * This should be used rather than using heap_insert directly in most places
+ * where we are modifying system catalogs.
+ */
+#define simple_heap_insert(relation, tup) \
+	heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL)
+
 extern HeapScanDesc heap_beginscan(Relation relation, Snapshot snapshot,
 			   int nkeys, ScanKey key);
 extern HeapScanDesc heap_beginscan_catalog(Relation relation, int nkeys,
@@ -176,7 +189,6 @@ extern bool heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_
 						MultiXactId cutoff_multi, Buffer buf);
 extern bool heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple);
 
-extern Oid	simple_heap_insert(Relation relation, HeapTuple tup);
 extern void simple_heap_delete(Relation relation, ItemPointer tid);
 extern void simple_heap_update(Relation relation, ItemPointer otid,
 				   HeapTuple tup);

Heikki Linnakangas

hlinnaka@iki.fi

over 7 years ago

In reply to: Andrey Klychkov (#1)

Re: [PATCH] Change simple_heap_insert() to a macro

On 12/10/2018 11:54, Andrey Klychkov wrote:

Studying another question I noticed a small point for optimization.

In the src/backend/access/heap/heapam.c we have the function:

- * simple_heap_insert - insert a tuple
- *
- * Currently, this routine differs from heap_insert only in supplying
- * a default command ID and not allowing access to the speedup options.
- *
- * This should be used rather than using heap_insert directly in most
places
- * where we are modifying system catalogs.
- */
-Oid
-simple_heap_insert(Relation relation, HeapTuple tup)
-{
- return heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL);
-}

I changed it to a macro. See the attached patch.

simple_heap_insert() is used in catalog updates and such. Does that have
any measurable performance impact?

- Heikki

Andrey Klychkov

aaklychkov@mail.ru

over 7 years ago

In reply to: Heikki Linnakangas (#2)

Re[2]: [PATCH] Change simple_heap_insert() to a macro

simple_heap_insert() is used in catalog updates and such. Does that have
any measurable performance impact?

I guess this change doesn't give us an incredible performance increase except there will no redundant function call.
I don't see any reasons against to use the proposed macro instead of this function.

Пятница, 12 октября 2018, 12:16 +03:00 от Heikki Linnakangas <hlinnaka@iki.fi>:

On 12/10/2018 11:54, Andrey Klychkov wrote:

Studying another question I noticed a small point for optimization.

In the src/backend/access/heap/heapam.c we have the function:

- * simple_heap_insert - insert a tuple
- *
- * Currently, this routine differs from heap_insert only in supplying
- * a default command ID and not allowing access to the speedup options.
- *
- * This should be used rather than using heap_insert directly in most
places
- * where we are modifying system catalogs.
- */
-Oid
-simple_heap_insert(Relation relation, HeapTuple tup)
-{
- return heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL);
-}

I changed it to a macro. See the attached patch.

simple_heap_insert() is used in catalog updates and such. Does that have
any measurable performance impact?

- Heikki

--
Regards,
Andrey Klychkov

Tom Lane

tgl@sss.pgh.pa.us

about 7 years ago

In reply to: Andrey Klychkov (#3)

Re: Re[2]: [PATCH] Change simple_heap_insert() to a macro

=?UTF-8?B?QW5kcmV5IEtseWNoa292?= <aaklychkov@mail.ru> writes:

simple_heap_insert() is used in catalog updates and such. Does that have
any measurable performance impact?

I guess this change doesn't give us an incredible performance increase except there will no redundant function call.
I don't see any reasons against to use the proposed macro instead of this function.

Well, by the same token, there's no reason in favor either.

In this particular case I'd vote against because the macro requires
more side-knowledge than the function call, ie GetCurrentCommandId
has to be in-scope for every caller. It's not hard to imagine
future changes that would make that problem worse.

In general, without a clearly measurable performance benefit,
changing functions into macros or inlines isn't a good idea.
The code churn poses hazards for back-patching, and there's
usually some physical code bloat due to more instructions being
needed at each call site.

regards, tom lane

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

about 7 years ago

In reply to: Andrey Klychkov (#3)

Re: [PATCH] Change simple_heap_insert() to a macro

On 12/10/2018 12:09, Andrey Klychkov wrote:

I don't see any reasons against to use the proposed macro instead of
this function.

Macros are weird and should be avoided if possible. If we were to do
this, it should be an inline function, I think. But I think it's not
useful here.

I think there have been enough votes against this that I'll close this
CF item.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services