Pavan Deolasee wrote:
> On Wed, Mar 29, 2017 at 3:42 AM, Alvaro Herrera <[email protected]>
> wrote:
>
> > I pushed 0002 after some makeup, since it's just cosmetic and not
> > controversial.
>
> Thanks. I think your patch of tracking interesting attributes seems ok too
> after the performance issue was addressed. Even though we can still improve
> that further, at least Mithun confirmed that there is no significant
> regression anymore and in fact for one artificial case, patch does better
> than even master.
Great, thanks. I pushed it, too. One optimization we could try is
using slot deform instead of repeated heap_getattr(). Patch is
attached. I haven't benchmarked it.
On top of that, but perhaps getting in the realm of excessive
complication, we could see if the bitmapset is a singleton, and if it is
then do heap_getattr without creating the slot. That'd require to have
a second copy of heap_tuple_attr_equals() that takes a HeapTuple instead
of a TupleTableSlot.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0c3e2b0..976de99 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -56,6 +56,7 @@
#include "access/xlogutils.h"
#include "catalog/catalog.h"
#include "catalog/namespace.h"
+#include "executor/tuptable.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "storage/bufmgr.h"
@@ -4337,7 +4338,7 @@ l2:
*/
static bool
heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum,
- HeapTuple tup1, HeapTuple tup2)
+ TupleTableSlot *tup1, TupleTableSlot
*tup2)
{
Datum value1,
value2;
@@ -4366,13 +4367,10 @@ heap_tuple_attr_equals(TupleDesc tupdesc, int attrnum,
}
/*
- * Extract the corresponding values. XXX this is pretty inefficient if
- * there are many indexed columns. Should HeapDetermineModifiedColumns
do
- * a single heap_deform_tuple call on each tuple, instead? But that
- * doesn't work for system columns ...
+ * Extract the corresponding values.
*/
- value1 = heap_getattr(tup1, attrnum, tupdesc, &isnull1);
- value2 = heap_getattr(tup2, attrnum, tupdesc, &isnull2);
+ value1 = slot_getattr(tup1, attrnum, &isnull1);
+ value2 = slot_getattr(tup2, attrnum, &isnull2);
/*
* If one value is NULL and other is not, then they are certainly not
@@ -4424,17 +4422,27 @@ HeapDetermineModifiedColumns(Relation relation,
Bitmapset *interesting_cols,
{
int attnum;
Bitmapset *modified = NULL;
+ TupleTableSlot *oldslot;
+ TupleTableSlot *newslot;
+
+ oldslot = MakeSingleTupleTableSlot(RelationGetDescr(relation));
+ ExecStoreTuple(oldtup, oldslot, InvalidBuffer, false);
+ newslot = MakeSingleTupleTableSlot(RelationGetDescr(relation));
+ ExecStoreTuple(newtup, newslot, InvalidBuffer, false);
while ((attnum = bms_first_member(interesting_cols)) >= 0)
{
attnum += FirstLowInvalidHeapAttributeNumber;
if (!heap_tuple_attr_equals(RelationGetDescr(relation),
- attnum,
oldtup, newtup))
+ attnum,
oldslot, newslot))
modified = bms_add_member(modified,
attnum - FirstLowInvalidHeapAttributeNumber);
}
+ ExecDropSingleTupleTableSlot(oldslot);
+ ExecDropSingleTupleTableSlot(newslot);
+
return modified;
}
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
