Re: 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(at)megazone23(dot)bigpanda(dot)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(at)megazone23(dot)bigpanda(dot)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(at)megazone23(dot)bigpanda(dot)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(at)megazone23(dot)bigpanda(dot)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(at)Yahoo(dot)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(at)megazone23(dot)bigpanda(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)megazone23(dot)bigpanda(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)megazone23(dot)bigpanda(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073