Thread: Is Patch Ok for deferred trigger disk queue?
Hi, I noticed the patch: http://archives.postgresql.org/pgsql-patches/2003-06/msg00366.php isn't in the patch queue. Is the patch OK? If not please say what is wrong with it. Thank you, - Stuart
On Mon, 30 Jun 2003, deststar wrote: > Hi, > I noticed the patch: > http://archives.postgresql.org/pgsql-patches/2003-06/msg00366.php > isn't in the patch queue. Is the patch OK? I think it was just that Bruce hasn't gotten to it. > If not please say what is wrong with it. I just checked out a new cvs copy and applied the patch, and did something like the following: create table a1(a int unique, b int, c int, unique(b,c)); insert into a1 values (1,1,1); create table a2(a int references a1(a), b int, c int, foreign key(b,c) references a1(b,c) initially deferred ); begin; insert into a2 values (1,1,1); insert into a2 select * from a2; [repeated a bunch of times until it'd be inserting 64k rows] and got an error on writing the disk event handle and a signal 11: ERROR: Can not open first disk event file handle for /usr/local/pgsql/data/base/17139/pgsql_tmp/pgsql_tmpdeftrig_555-000000001 The connection to the server was lost. Attempting reset: LOG: server process (pid 32125) was terminated by signal 11 The backtrace from the core looked like: #0 0x42062867 in fclose@@GLIBC_2.1 () from /lib/i686/libc.so.6 #1 0x080d61a8 in deferredTriggerClean () at trigger.c:1864 #2 0x080d728e in DeferredTriggerAbortXact () at trigger.c:2642 #3 0x0808c214 in AbortTransaction () at xact.c:1042 #4 0x08141332 in PostgresMain (argc=4, argv=0x826a2f0, username=0x826a2c0 "sszabo") at postgres.c:2610 #5 0x0812320e in BackendFork (port=0x8277080) at postmaster.c:2471 #6 0x08122d1e in BackendStartup (port=0x8277080) at postmaster.c:2118 #7 0x081218ab in ServerLoop () at postmaster.c:1090 #8 0x08121358 in PostmasterMain (argc=3, argv=0x82693e0) at postmaster.c:872 #9 0x080f9e30 in main (argc=3, argv=0xbffffa94) at main.c:211 #10 0x420158f7 in __libc_start_main () from /lib/i686/libc.so.6 looks like it was passing a NULL file handle if it couldn't be opened.
On Mon, 30 Jun 2003, Stephan Szabo wrote: > On Mon, 30 Jun 2003, deststar wrote: > > > Hi, > > I noticed the patch: > > http://archives.postgresql.org/pgsql-patches/2003-06/msg00366.php > > isn't in the patch queue. Is the patch OK? > > I think it was just that Bruce hasn't gotten to it. > > > If not please say what is wrong with it. > > I just checked out a new cvs copy and applied the patch, and did something > like the following: > create table a1(a int unique, b int, c int, unique(b,c)); > insert into a1 values (1,1,1); > create table a2(a int references a1(a), b int, c int, foreign > key(b,c) references a1(b,c) initially deferred ); > begin; > insert into a2 values (1,1,1); > insert into a2 select * from a2; > [repeated a bunch of times until it'd be inserting 64k rows] > > and got an error on writing the disk event handle and a signal 11: > > ERROR: Can not open first disk event file handle for > /usr/local/pgsql/data/base/17139/pgsql_tmp/pgsql_tmpdeftrig_555-000000001 > The connection to the server was lost. Attempting reset: LOG: server > process (pid 32125) was terminated by signal 11 > > The backtrace from the core looked like: > #0 0x42062867 in fclose@@GLIBC_2.1 () from /lib/i686/libc.so.6 > #1 0x080d61a8 in deferredTriggerClean () at trigger.c:1864 > #2 0x080d728e in DeferredTriggerAbortXact () at trigger.c:2642 > #3 0x0808c214 in AbortTransaction () at xact.c:1042 > #4 0x08141332 in PostgresMain (argc=4, argv=0x826a2f0, username=0x826a2c0 > "sszabo") at postgres.c:2610 > #5 0x0812320e in BackendFork (port=0x8277080) at postmaster.c:2471 > #6 0x08122d1e in BackendStartup (port=0x8277080) at postmaster.c:2118 > #7 0x081218ab in ServerLoop () at postmaster.c:1090 > #8 0x08121358 in PostmasterMain (argc=3, argv=0x82693e0) at > postmaster.c:872 > #9 0x080f9e30 in main (argc=3, argv=0xbffffa94) at main.c:211 > #10 0x420158f7 in __libc_start_main () from /lib/i686/libc.so.6 > > looks like it was passing a NULL file handle if it couldn't be opened. The open error seems to have been errno=13 (EACCES). I was able to get past that by changing pgsql_tmp's permissions to 700 rather than 600. The 64k insert one worked, but the next insert ... select failed with: ERROR: Attempt to read from disk deferred trigger queue before initialisation. --- As a side question, it looks to me that the code stores the first trigger records in memory and then after some point starts storing all new records on disk. Is this correct? I'd wonder if that's really what you want in general, since I'd think that the earliest ones are the ones you're least likely to need until end of transaction (or set constraints in the fk case) whereas the most recent ones are possibly going to be immediate triggers which you're going to need as soon as the statement is done.
Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
> As a side question, it looks to me that the code stores the first trigger
> records in memory and then after some point starts storing all new records
> on disk. Is this correct? I'd wonder if that's really what you want in
> general, since I'd think that the earliest ones are the ones you're least
> likely to need until end of transaction (or set constraints in the fk
> case) whereas the most recent ones are possibly going to be immediate
> triggers which you're going to need as soon as the statement is done.
Good point. It would be better to push out stuff from the head of the
queue, hoping that stuff near the end might never need to be written
at all.
regards, tom lane
Tom Lane wrote: > Stephan Szabo <sszabo@megazone23.bigpanda.com> writes: > >>As a side question, it looks to me that the code stores the first trigger >>records in memory and then after some point starts storing all new records >>on disk. Is this correct? I'd wonder if that's really what you want in >>general, since I'd think that the earliest ones are the ones you're least >>likely to need until end of transaction (or set constraints in the fk >>case) whereas the most recent ones are possibly going to be immediate >>triggers which you're going to need as soon as the statement is done. > > > Good point. It would be better to push out stuff from the head of the > queue, hoping that stuff near the end might never need to be written > at all. > > regards, tom lane Hmmm.... I see your point. I will change the patch to write the head to disk and reenter when the development branch splits off. Also I've noticed that there is an fd.h which has file routines which I should be using rather than the stdio routines. I will also clean up those errors. Thank you, - Stuart
On Tue, 1 Jul 2003, Stuart wrote: > Tom Lane wrote: > > > Stephan Szabo <sszabo@megazone23.bigpanda.com> writes: > > > >>As a side question, it looks to me that the code stores the first trigger > >>records in memory and then after some point starts storing all new records > >>on disk. Is this correct? I'd wonder if that's really what you want in > >>general, since I'd think that the earliest ones are the ones you're least > >>likely to need until end of transaction (or set constraints in the fk > >>case) whereas the most recent ones are possibly going to be immediate > >>triggers which you're going to need as soon as the statement is done. > > > > > > Good point. It would be better to push out stuff from the head of the > > queue, hoping that stuff near the end might never need to be written > > at all. > > > > regards, tom lane > Hmmm.... I see your point. I will change the patch to write the head to > disk and reenter when the development branch splits off. > Also I've noticed that there is an fd.h which has file routines which I > should be using rather than the stdio routines. > I will also clean up those errors. Hmm, it also looks like the original patch broke deferred foreign keys. You'd not have noticed it since I'd forgotten the case in question for the foreign key regression tests. I'm going to make a patch for the tests since we should be testing that in any case.
Stuart wrote: > Tom Lane wrote: > >> Stephan Szabo <sszabo@megazone23.bigpanda.com> writes: >> >>>As a side question, it looks to me that the code stores the first trigger >>>records in memory and then after some point starts storing all new records >>>on disk. Is this correct? I'd wonder if that's really what you want in >>>general, since I'd think that the earliest ones are the ones you're least >>>likely to need until end of transaction (or set constraints in the fk >>>case) whereas the most recent ones are possibly going to be immediate >>>triggers which you're going to need as soon as the statement is done. >> >> >> Good point. It would be better to push out stuff from the head of the >> queue, hoping that stuff near the end might never need to be written >> at all. >> >> regards, tom lane > Hmmm.... I see your point. I will change the patch to write the head to > disk and reenter when the development branch splits off. > Also I've noticed that there is an fd.h which has file routines which I > should be using rather than the stdio routines. > I will also clean up those errors. While you are still at it, can you make the arbitrarily choosen trigger queue size a config parameter? It is much easier to do tuning without the need to recompile the backend. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Stuart, were are on this patch? Seems we need GUC additions, though I can do that for you, and changes to write the head to disk. Was that completed? --------------------------------------------------------------------------- Stuart wrote: > Tom Lane wrote: > > > Stephan Szabo <sszabo@megazone23.bigpanda.com> writes: > > > >>As a side question, it looks to me that the code stores the first trigger > >>records in memory and then after some point starts storing all new records > >>on disk. Is this correct? I'd wonder if that's really what you want in > >>general, since I'd think that the earliest ones are the ones you're least > >>likely to need until end of transaction (or set constraints in the fk > >>case) whereas the most recent ones are possibly going to be immediate > >>triggers which you're going to need as soon as the statement is done. > > > > > > Good point. It would be better to push out stuff from the head of the > > queue, hoping that stuff near the end might never need to be written > > at all. > > > > regards, tom lane > Hmmm.... I see your point. I will change the patch to write the head to > disk and reenter when the development branch splits off. > Also I've noticed that there is an fd.h which has file routines which I > should be using rather than the stdio routines. > I will also clean up those errors. > Thank you, > - Stuart > > > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
I assume this will not be completed for 7.4. I will keep the emails for 7.5. One idea I had was to use the existing sort_mem parameter to control when to force the deferred trigger queue to disk --- it doesn't have anything to do with sorting, but it does have the same purpose, to force thing to disk when we consume enough RAM. --------------------------------------------------------------------------- Bruce Momjian wrote: > > Stuart, were are on this patch? Seems we need GUC additions, though I > can do that for you, and changes to write the head to disk. Was that > completed? > > --------------------------------------------------------------------------- > > Stuart wrote: > > Tom Lane wrote: > > > > > Stephan Szabo <sszabo@megazone23.bigpanda.com> writes: > > > > > >>As a side question, it looks to me that the code stores the first trigger > > >>records in memory and then after some point starts storing all new records > > >>on disk. Is this correct? I'd wonder if that's really what you want in > > >>general, since I'd think that the earliest ones are the ones you're least > > >>likely to need until end of transaction (or set constraints in the fk > > >>case) whereas the most recent ones are possibly going to be immediate > > >>triggers which you're going to need as soon as the statement is done. > > > > > > > > > Good point. It would be better to push out stuff from the head of the > > > queue, hoping that stuff near the end might never need to be written > > > at all. > > > > > > regards, tom lane > > Hmmm.... I see your point. I will change the patch to write the head to > > disk and reenter when the development branch splits off. > > Also I've noticed that there is an fd.h which has file routines which I > > should be using rather than the stdio routines. > > I will also clean up those errors. > > Thank you, > > - Stuart > > > > > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 6: Have you searched our list archives? > > > > http://archives.postgresql.org > > > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (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 > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian wrote: > I assume this will not be completed for 7.4. I will keep the emails for > 7.5. > > One idea I had was to use the existing sort_mem parameter to control > when to force the deferred trigger queue to disk --- it doesn't have > anything to do with sorting, but it does have the same purpose, to force > thing to disk when we consume enough RAM. > > --------------------------------------------------------------------------- > > Bruce Momjian wrote: > >>Stuart, were are on this patch? Seems we need GUC additions, though I >>can do that for you, and changes to write the head to disk. Was that >>completed? >> >>--------------------------------------------------------------------------- >> >>Stuart wrote: >> >>>Tom Lane wrote: >>> >>> >>>>Stephan Szabo <sszabo@megazone23.bigpanda.com> writes: >>>> >>>> >>>>>As a side question, it looks to me that the code stores the first trigger >>>>>records in memory and then after some point starts storing all new records >>>>>on disk. Is this correct? I'd wonder if that's really what you want in >>>>>general, since I'd think that the earliest ones are the ones you're least >>>>>likely to need until end of transaction (or set constraints in the fk >>>>>case) whereas the most recent ones are possibly going to be immediate >>>>>triggers which you're going to need as soon as the statement is done. >>>> >>>> >>>>Good point. It would be better to push out stuff from the head of the >>>>queue, hoping that stuff near the end might never need to be written >>>>at all. >>>> >>>> regards, tom lane >>> >>>Hmmm.... I see your point. I will change the patch to write the head to >>>disk and reenter when the development branch splits off. >>>Also I've noticed that there is an fd.h which has file routines which I >>>should be using rather than the stdio routines. >>>I will also clean up those errors. >>>Thank you, >>>- Stuart >>> Sorry for the tardiness in replying, I've been away for the past week or so. I didn't intend for 7.4 partly because I knew I'd be away & partly because I had seen there was a problem I hadn't realised with the previous patch and didn't want to submit something that may not be stable just before beta. Currently it compiles but there are some, er, issues - shouldn't take to long to fix but it might not be till wednesday as I've got a bit of a backlog to get through. I could use sortmem, but if this is to be the case maybe there should be a change the call it something like max_local_mem with a way to register that you are using it. Maybe the memory allocs could automatically add to it and remove as memory is assigned. Alternativly just make a global to record the memory currently used by interested parties (currently the trigger & sortmem I'd guess). The only trouble with this that I can see is that the first one to claim the memory may claim it all, leaving nothing for the other. I'll carry on using the dedicated guc variable for the moment as I can't really see the correct way to solve this cleanly. regards, - Stuart
Stuart wrote: > Sorry for the tardiness in replying, I've been away for the past week or so. > I didn't intend for 7.4 partly because I knew I'd be away & partly > because I had seen there was a problem I hadn't realised with the > previous patch and didn't want to submit something that may not be > stable just before beta. Currently it compiles but there are some, er, > issues - shouldn't take to long to fix but it might not be till > wednesday as I've got a bit of a backlog to get through. > I could use sortmem, but if this is to be the case maybe there should be > a change the call it something like max_local_mem with a way to register > that you are using it. Maybe the memory allocs could automatically add > to it and remove as memory is assigned. Alternativly just make a global > to record the memory currently used by interested parties (currently the > trigger & sortmem I'd guess). The only trouble with this that I can see > is that the first one to claim the memory may claim it all, leaving > nothing for the other. I'll carry on using the dedicated guc variable > for the moment as I can't really see the correct way to solve this cleanly. > regards, OK, we can do the trigger queue file for 7.5. The issue with sortmem is that its effect is to spill a sort out to file when it gets too large, the same for the trigger queue representation. We could rename it, but because it is _mostly_ used for sorts, we would probably keep the name the same and just mention the trigger queue effect in the docs. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073