Обсуждение: [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
> 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>:
> 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
=?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