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

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

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

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>:

=?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

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