Re: [HACKERS] Open 7.3 items: heap tuple header
On Fri, 16 Aug 2002 01:05:07 -0400 (EDT), Bruce Momjian [EMAIL PROTECTED] wrote: P O S T G R E S Q L 7 . 3 O P E NI T E M S improve macros in new tuple header code (Manfred) ISTM there's no consensus about what improve means. I tried to start discussing this after my vacation, but apparently people had better things to do. On Wed, 07 Aug 2002 16:16:14 +0200, I wrote (Heap tuple header issues): :. Transaction and command ids, performance : :I offered to provide cheaper versions of GetCmin and GetCmax to be :used by the tqual routines. These version would reduce additional CPU :work from two infomask compares to one. Is this still considered an :issue? However, I don't think this would lead to any measurable difference. :. Transaction and command ids, robustness : :I'm still of the opinion that putting *more* knowledge into the SetXxx :macros is the way to go. The runaway INSERT bug could as well have :been fixed by changing SetCmax to do nothing, if HEAP_XMAX_INVALID is :set, and changing SetXmaxInvalid to set HEAP_XMAX_INVALID. Likewise I :would change SetXmax to set HEAP_XMAX_INVALID, if xid == :InvalidTransactionId, or reset it, if != (not sure about this). Same :for SetXminInvalid and SetXmin. This is the main point of disagreement: Tom Lane wants lighter macros, I want heavier macros. Which direction shall we go? :Further, I'll try to build a regression test using statement timeout :to detect runaway INSERT/UPDATE (the so called halloween problem). This won't hurt anyway. I'll start working on this. BTW, my changes have been criticized with words like vague unease, zero confidence, very obviously not robust, do not trust the current code at all etc., while from day one all my patches have passed all regression tests. This makes me wonder whether there is something wrong with the regression tests ... :. Oids : :I was a bit surprised that these patches went in so smoothly, must be :due to the fact that there was a lot of work to do at that time. :Personnally I feel that these changes are more dangerous than the :Xmin/Cid/Xmax patches; and I ask the hackers to review especially :part 2, which contains changes to the executor and even to bootstrap. :. Oids, t_infomask : :There has been no comment from a tool developer. :. Oids, heap_getsysattr : :We thought that a TupleDesc parameter would have to be added for this :function. However, tests showed that heap_getsysattr is not called to :get the oid, when the tuple doesn't have an oid: ERROR: Attribute :'oid' not found. :. Oids, micro tuning : :There are a few places, where storing an oid in a local variable might :be a little faster than fetching it several times from a heap tuple :header. However, I don't think this would lead to any measurable difference. :. Overall performance : :If Joe Conway can be talked into running OSDB benchmarks with old and :new heap tuple header format, I'll provide patches and instructions to :easily switch between versions. Or, Joe, can you tell me, what I need :to have and need to do to set up a benchmarking environment? With Joe's help (thanks again, Joe) I've managed to setup a benchmarking environment and I am continuously testing different configurations for a week now. There are issues with OSDB which I plan to bring up later when things cool down, but a first anawhat seems to show that with the reduced heap tuple header size we get a speed improvement of up to 3%, especially when the database is significantly larger than system memory. When the database size is only a small fraction of available memory, results vary so widely that I cannot tell whether the new heap tuple macros are a loss or a win. : :. CVS : :There have been a lot of CVS broken messages in the past few days. :When I tried : cvs -z3 log heapam.c :I got :| cvs server: failed to create lock directory for :`/projects/cvsroot/pgsql/src/backend/access/heap' :(/projects/cvsroot/pgsql/src/backend/access/heap/#cvs.lock): No such file or directory :| cvs server: failed to obtain dir lock in repository :`/projects/cvsroot/pgsql/src/backend/access/heap' :| cvs [server aborted]: read lock failed - giving up : :Is this a temporary problem or did a miss any planned changes? AFAIK I have to re-checkout everything. Servus Manfred ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] Open 7.3 items: heap tuple header
Manfred Koizar wrote: On Fri, 16 Aug 2002 01:05:07 -0400 (EDT), Bruce Momjian [EMAIL PROTECTED] wrote: P O S T G R E S Q L 7 . 3 O P E NI T E M S improve macros in new tuple header code (Manfred) ISTM there's no consensus about what improve means. I tried to start discussing this after my vacation, but apparently people had better things to do. OK. On Wed, 07 Aug 2002 16:16:14 +0200, I wrote (Heap tuple header issues): :. Transaction and command ids, performance : :I offered to provide cheaper versions of GetCmin and GetCmax to be :used by the tqual routines. These version would reduce additional CPU :work from two infomask compares to one. Is this still considered an :issue? However, I don't think this would lead to any measurable difference. Yep. :. Transaction and command ids, robustness : :I'm still of the opinion that putting *more* knowledge into the SetXxx :macros is the way to go. The runaway INSERT bug could as well have :been fixed by changing SetCmax to do nothing, if HEAP_XMAX_INVALID is :set, and changing SetXmaxInvalid to set HEAP_XMAX_INVALID. Likewise I :would change SetXmax to set HEAP_XMAX_INVALID, if xid == :InvalidTransactionId, or reset it, if != (not sure about this). Same :for SetXminInvalid and SetXmin. This is the main point of disagreement: Tom Lane wants lighter macros, I want heavier macros. Which direction shall we go? Could you or Tom explain that in a way that others could understand. My guess is that you want the sanity checks in the macros, and Tom wants more of them in the main code? :Further, I'll try to build a regression test using statement timeout :to detect runaway INSERT/UPDATE (the so called halloween problem). This won't hurt anyway. I'll start working on this. BTW, my changes have been criticized with words like vague unease, zero confidence, very obviously not robust, do not trust the current code at all etc., while from day one all my patches have passed all regression tests. I totally agree with you here. You code has been great, and it did something (reduce tuple size) that no one else thought possible. This makes me wonder whether there is something wrong with the regression tests ... However, I should add that the regression tests test only a small subset of how the database has to operate. There are so many bizarre conditions that we can't test them all. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] Open 7.3 items: heap tuple header
On Fri, 16 Aug 2002 12:25:37 -0400 (EDT), Bruce Momjian [EMAIL PROTECTED] wrote: Manfred Koizar wrote: This is the main point of disagreement: Tom Lane wants lighter macros, I want heavier macros. Which direction shall we go? Could you or Tom explain that in a way that others could understand. I'm not sure we understand each other :-) My guess is that you want the sanity checks in the macros, and Tom wants more of them in the main code? I guess we agree on having the sanity checks in the macros. It's more on what the macros are allowed to change and what decisions they are allowed to take. Do the following quotes make it clearer? :On Sat, 20 Jul 2002 16:27:14 -0400, Tom Lane [EMAIL PROTECTED] :wrote: :I'd recommend redesigning the HeapTupleHeaderSet macros so that they :do not do any setting of t_infomask bits, or even take any conditional :action based on them, to which I replied on Sun, 21 Jul 2002 23:00:22 +0200: :The HEAP_XMIN_IS_XMAX bit is exclusively managed by these macros. :Pulling the handling of this bit out of the macros and spreading it to :the places, where the macros are used, cannot make the whole thing :more robust. This would mean, the caller had to decide whether to :store Cmax into t_cid or t_xmax... Tom: : but solely Assert() that the bits are already :in the appropriate state to allow storing of the value to be stored. :Then, all uses have to be checked to ensure that t_infomask is coerced :into the right state *before* doing HeapTupleHeaderSetFoo. me: :Apart from HEAP_XMIN_IS_XMAX this was my intention; we already do :this with HEAP_MOVED. I could add an assertion to SetCmax: : Assert(!((tup)-t_infomask HEAP_XMAX_INVALID)); : :OTOH I thought about putting *more* logic into the macros to make :their use less fragile. For example SetXmaxInvalid could set the :HEAP_XMAX_INVALID bit, likewise SetCminInvalid with XMIN_INVALID. BTW, my changes have been criticized with words like vague unease, zero confidence, very obviously not robust, do not trust the current code at all etc., while from day one all my patches have passed all regression tests. I totally agree with you here. You code has been great, and it did something (reduce tuple size) that no one else thought possible. Thanks a lot! But I was not fishing for compliments, I was just preparing for the next sentence: There is something wrong with the regression tests. In fact vague unease more than once was a proper description for my own feelings when I posted a patch, because I didn't know whether I could trust the tests. If core developers share this unease, I find myself in good company. This makes me wonder whether there is something wrong with the regression tests ... However, I should add that the regression tests test only a small subset of how the database has to operate. So, please, could you add to TODO: * Add more regression tests There are so many bizarre conditions that we can't test them all. When Tom Lane recently posted a way to reproduce a bug (No one parent tuple was found, CASE 1) I thought about how to add this case to the regression tests, but we have no vehicle for testing concurrent transactions. TODO: * Build a test vehicle for concurrent transactions * Add even more regression tests Servus Manfred ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
