Re: [HACKERS] [patch] executor and slru dtrace probes
Greg Smith píše v út 15. 12. 2009 v 12:10 -0500: Please send that updated version, and let's keep working on this into the next CommitFest, where it will be in the front of the queue rather than how it ended up at the tail of this one just based on its submission date. You're not really getting a fair chunk of time here between your review and the end here because of problems lining up reviewer time, that shouldn't happen next time. Make sense. Zdenek -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
Zdenek Kotala wrote: Bernd Helmle píše v po 14. 12. 2009 v 20:42 +0100: Oh, and i was under the opinion the last discussions were about executor probes only (note the patch is split up into two parts now, SLRU and executor probes). The latter won't be fixed, but it seems the SLRU part at least is ready. I would like to add SimpleLruInit probe. I'm busy with PG packaging, but I hope that I will send updated version tomorrow. I'd like to see as many of these DTrace probes as possible make it into 8.5, and it's great that you've picked up these older ones and updated them. It looks like a lot of progress was made on actually measuring the overhead of adding these probes in even when they're not enabled, which is good. But I'm afraid we're already out of time for this one if you're still tweaking the probes here. With a functional change like that, our normal process at this point would be to have the reviewer re-evaluate things before they head to a committer, and I don't feel like this patch is quite at 100% yet--in particular, the probe documentation is improving but still a bit rough. I don't feel like we're quite ready to mark this one for commit for this one, and today we really want to clear the queue for things for committers to deal with. Please send that updated version, and let's keep working on this into the next CommitFest, where it will be in the front of the queue rather than how it ended up at the tail of this one just based on its submission date. You're not really getting a fair chunk of time here between your review and the end here because of problems lining up reviewer time, that shouldn't happen next time. -- Greg Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com
Re: [HACKERS] [patch] executor and slru dtrace probes
--On 15. Dezember 2009 12:10:09 -0500 Greg Smith g...@2ndquadrant.com wrote: But I'm afraid we're already out of time for this one if you're still tweaking the probes here. With a functional change like that, our normal process at this point would be to have the reviewer re-evaluate things before they head to a committer, and I don't feel like this patch is quite at 100% yet--in particular, the probe documentation is improving but still a bit rough. I don't feel like we're quite ready to mark this one for commit for this one, and today we really want to clear the queue for things for committers to deal with. Please send that updated version, and let's keep working on this into the next CommitFest, where it will be in the front of the queue rather than how it ended up at the tail of this one just based on its submission date. You're not really getting a fair chunk of time here between your review and the end here because of problems lining up reviewer time, that shouldn't happen next time. That seems reasonable. I hope i could contribute something, even this was the first time i got my hands on reviewing this DTrace thingie. -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
--On 10. Dezember 2009 16:49:50 +0100 Zdenek Kotala zdenek.kot...@sun.com wrote: You need to determine which SLRU is used. Because SLRUs are initialized during startup pointer should be same in all backends. If I think more about it. Maybe it could be goot to add probe also into SimpleLruInit to catch name of SLRUs. Hi Zdenek, do you plan to work on this further? I was about to mark the SLRU probes as ready for committer... -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
On Mon, Dec 14, 2009 at 7:19 AM, Bernd Helmle maili...@oopsware.de wrote: --On 10. Dezember 2009 16:49:50 +0100 Zdenek Kotala zdenek.kot...@sun.com wrote: You need to determine which SLRU is used. Because SLRUs are initialized during startup pointer should be same in all backends. If I think more about it. Maybe it could be goot to add probe also into SimpleLruInit to catch name of SLRUs. Hi Zdenek, do you plan to work on this further? I was about to mark the SLRU probes as ready for committer... Since the author has pretty much admitted he didn't fix any of the issues that were raised by the last committer review, I'm a little confused about why you're asking for another one. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
--On 14. Dezember 2009 07:49:34 -0500 Robert Haas robertmh...@gmail.com wrote: Since the author has pretty much admitted he didn't fix any of the issues that were raised by the last committer review, I'm a little confused about why you're asking for another one. It wasn't clear to me what Zdenek meant with If I think about it. -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
--On 14. Dezember 2009 20:33:12 +0100 Bernd Helmle maili...@oopsware.de wrote: Since the author has pretty much admitted he didn't fix any of the issues that were raised by the last committer review, I'm a little confused about why you're asking for another one. It wasn't clear to me what Zdenek meant with If I think about it. Oh, and i was under the opinion the last discussions were about executor probes only (note the patch is split up into two parts now, SLRU and executor probes). The latter won't be fixed, but it seems the SLRU part at least is ready. -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
Bernd Helmle píše v po 14. 12. 2009 v 20:42 +0100: --On 14. Dezember 2009 20:33:12 +0100 Bernd Helmle maili...@oopsware.de wrote: Since the author has pretty much admitted he didn't fix any of the issues that were raised by the last committer review, I'm a little confused about why you're asking for another one. It wasn't clear to me what Zdenek meant with If I think about it. Oh, and i was under the opinion the last discussions were about executor probes only (note the patch is split up into two parts now, SLRU and executor probes). The latter won't be fixed, but it seems the SLRU part at least is ready. I would like to add SimpleLruInit probe. I'm busy with PG packaging, but I hope that I will send updated version tomorrow. Zdenek -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
--On 8. Dezember 2009 11:10:44 +0100 Zdenek Kotala zdenek.kot...@sun.com wrote: If you think that it is better I could split patch into two separate patches and both can be reviewed separately. I split up this patch into two separate patches: one for SLRU and one for the executor probes. I've done some documentation on the SLRU part, maybe you can look over it (to make sure i didn't break anything). I left the executor probes out of the documentation for now, since it seems not to be clear how they would manifest. Out of curiosity: Why do we want to pass the SlruCtl pointer down to the probes? I don't understand what those probes are going to do with those pointers, can you explain? -- Thanks Bernd dtrace_executor_probes.patch Description: application/text dtrace_slru_probes.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
Dne 10.12.09 15:51, Bernd Helmle napsal(a): --On 8. Dezember 2009 11:10:44 +0100 Zdenek Kotala zdenek.kot...@sun.com wrote: If you think that it is better I could split patch into two separate patches and both can be reviewed separately. I split up this patch into two separate patches: one for SLRU and one for the executor probes. I've done some documentation on the SLRU part, maybe you can look over it (to make sure i didn't break anything). I left the executor probes out of the documentation for now, since it seems not to be clear how they would manifest. Out of curiosity: Why do we want to pass the SlruCtl pointer down to the probes? I don't understand what those probes are going to do with those pointers, can you explain? You need to determine which SLRU is used. Because SLRUs are initialized during startup pointer should be same in all backends. If I think more about it. Maybe it could be goot to add probe also into SimpleLruInit to catch name of SLRUs. Zdenek -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
--On 9. Dezember 2009 19:08:07 -0500 Theo Schlossnagle je...@omniti.com wrote: Now, there was some indication that there was a better place to probe that would be more comprehensive -- that should be addressed. For now there exists no consensus where they should go in. Tom pointed out various issues with ExecProcNode() and he's worried about the performance penalty those probes might introduce. I admit I'm not very experienced with dtrace, but maybe some worries exists because an expensive instrumented executor probe can cause forged results? -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
On Dec 8, 2009, at 5:10 AM, Zdenek Kotala wrote: Dne 8.12.09 00:27, Bernd Helmle napsal(a): --On 13. November 2009 23:29:41 +0100 Zdenek Kotala zdenek.kot...@sun.com wrote: t contains two DTrace probe groups. One is related to monitoring SLRU and second is about executor nodes. I merged it with the head. Original end of mail thread is here: http://archives.postgresql.org/pgsql-hackers/2009-04/msg00148.php I've started to review this. It seems to me the attached patch wasn't adjusted or discussed again to address Tom's complaints? At least the executor probes contained here hold still the same issues mentioned by Tom in the discussion linked here. I did not make any change. I only revival patch and merge it with head. I think that SLRU probes are OK and acceptable. Tom's issues with executor probes are still there and I expect discussion about them. IIRC Theo uses these probes in production. If you think that it is better I could split patch into two separate patches and both can be reviewed separately. I suppose I see it as a simple thing. The probes have no performance impact when they are not instrumented. I've used them on rare occasion to understand which exec nodes are causing which disk accesses. Seemed pretty darn useful at the time. There is (of course) some performance overhead when they are enabled, but that is intentionally performed by the operator, so it seems like a non-issue. Now, there was some indication that there was a better place to probe that would be more comprehensive -- that should be addressed. -- Theo Schlossnagle Esoteric Curio -- http://lethargy.org/ OmniTI Computer Consulting, Inc. -- http://omniti.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
Dne 8.12.09 00:27, Bernd Helmle napsal(a): --On 13. November 2009 23:29:41 +0100 Zdenek Kotala zdenek.kot...@sun.com wrote: t contains two DTrace probe groups. One is related to monitoring SLRU and second is about executor nodes. I merged it with the head. Original end of mail thread is here: http://archives.postgresql.org/pgsql-hackers/2009-04/msg00148.php I've started to review this. It seems to me the attached patch wasn't adjusted or discussed again to address Tom's complaints? At least the executor probes contained here hold still the same issues mentioned by Tom in the discussion linked here. I did not make any change. I only revival patch and merge it with head. I think that SLRU probes are OK and acceptable. Tom's issues with executor probes are still there and I expect discussion about them. IIRC Theo uses these probes in production. If you think that it is better I could split patch into two separate patches and both can be reviewed separately. thanks Zdenek -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] executor and slru dtrace probes
--On 13. November 2009 23:29:41 +0100 Zdenek Kotala zdenek.kot...@sun.com wrote: t contains two DTrace probe groups. One is related to monitoring SLRU and second is about executor nodes. I merged it with the head. Original end of mail thread is here: http://archives.postgresql.org/pgsql-hackers/2009-04/msg00148.php I've started to review this. It seems to me the attached patch wasn't adjusted or discussed again to address Tom's complaints? At least the executor probes contained here hold still the same issues mentioned by Tom in the discussion linked here. -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [patch] executor and slru dtrace probes
I attached patch which was already sent on february/april, but it was
lost in time. It is originally from Robert Lor and Theo Schlossnagle.
It contains two DTrace probe groups. One is related to monitoring SLRU
and second is about executor nodes.
I merged it with the head.
Original end of mail thread is here:
http://archives.postgresql.org/pgsql-hackers/2009-04/msg00148.php
Zdenek
diff -r 68b8827f4738 src/backend/access/transam/slru.c
--- a/src/backend/access/transam/slru.c Fri Nov 13 11:17:04 2009 +
+++ b/src/backend/access/transam/slru.c Fri Nov 13 23:29:14 2009 +0100
@@ -57,6 +57,7 @@
#include storage/fd.h
#include storage/shmem.h
#include miscadmin.h
+#include pg_trace.h
/*
@@ -372,6 +373,7 @@
{
SlruShared shared = ctl-shared;
+ TRACE_POSTGRESQL_SLRU_READPAGE_START((uintptr_t)ctl, pageno, write_ok, xid);
/* Outer loop handles restart if we must wait for someone else's I/O */
for (;;)
{
@@ -399,6 +401,7 @@
}
/* Otherwise, it's ready to use */
SlruRecentlyUsed(shared, slotno);
+ TRACE_POSTGRESQL_SLRU_READPAGE_DONE(slotno);
return slotno;
}
@@ -446,6 +449,7 @@
SlruReportIOError(ctl, pageno, xid);
SlruRecentlyUsed(shared, slotno);
+ TRACE_POSTGRESQL_SLRU_READPAGE_DONE(slotno);
return slotno;
}
}
@@ -470,6 +474,8 @@
SlruShared shared = ctl-shared;
int slotno;
+ TRACE_POSTGRESQL_SLRU_READPAGE_READONLY((uintptr_t)ctl, pageno, xid);
+
/* Try to find the page while holding only shared lock */
LWLockAcquire(shared-ControlLock, LW_SHARED);
@@ -511,6 +517,8 @@
int pageno = shared-page_number[slotno];
bool ok;
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_START((uintptr_t)ctl, pageno, slotno);
+
/* If a write is in progress, wait for it to finish */
while (shared-page_status[slotno] == SLRU_PAGE_WRITE_IN_PROGRESS
shared-page_number[slotno] == pageno)
@@ -525,7 +533,10 @@
if (!shared-page_dirty[slotno] ||
shared-page_status[slotno] != SLRU_PAGE_VALID ||
shared-page_number[slotno] != pageno)
+ {
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_DONE();
return;
+ }
/*
* Mark the slot write-busy, and clear the dirtybit. After this point, a
@@ -569,6 +580,8 @@
/* Now it's okay to ereport if we failed */
if (!ok)
SlruReportIOError(ctl, pageno, InvalidTransactionId);
+
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_DONE();
}
/*
@@ -593,6 +606,8 @@
SlruFileName(ctl, path, segno);
+ TRACE_POSTGRESQL_SLRU_READPAGE_PHYSICAL_START((uintptr_t)ctl, path, pageno, slotno);
+
/*
* In a crash-and-restart situation, it's possible for us to receive
* commands to set the commit status of transactions whose bits are in
@@ -607,6 +622,7 @@
{
slru_errcause = SLRU_OPEN_FAILED;
slru_errno = errno;
+ TRACE_POSTGRESQL_SLRU_READPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
return false;
}
@@ -614,6 +630,7 @@
(errmsg(file \%s\ doesn't exist, reading as zeroes,
path)));
MemSet(shared-page_buffer[slotno], 0, BLCKSZ);
+ TRACE_POSTGRESQL_SLRU_READPAGE_PHYSICAL_DONE(true, -1, -1);
return true;
}
@@ -622,6 +639,7 @@
slru_errcause = SLRU_SEEK_FAILED;
slru_errno = errno;
close(fd);
+ TRACE_POSTGRESQL_SLRU_READPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
return false;
}
@@ -631,6 +649,7 @@
slru_errcause = SLRU_READ_FAILED;
slru_errno = errno;
close(fd);
+ TRACE_POSTGRESQL_SLRU_READPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
return false;
}
@@ -638,9 +657,12 @@
{
slru_errcause = SLRU_CLOSE_FAILED;
slru_errno = errno;
+ TRACE_POSTGRESQL_SLRU_READPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
return false;
}
+ TRACE_POSTGRESQL_SLRU_READPAGE_PHYSICAL_DONE(true, -1, -1);
+
return true;
}
@@ -668,6 +690,8 @@
char path[MAXPGPATH];
int fd = -1;
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_PHYSICAL_START((uintptr_t)ctl, pageno, slotno);
+
/*
* Honor the write-WAL-before-data rule, if appropriate, so that we do not
* write out data before associated WAL records. This is the same action
@@ -753,6 +777,7 @@
{
slru_errcause = SLRU_OPEN_FAILED;
slru_errno = errno;
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
return false;
}
@@ -781,6 +806,7 @@
slru_errno = errno;
if (!fdata)
close(fd);
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
return false;
}
@@ -794,6 +820,7 @@
slru_errno = errno;
if (!fdata)
close(fd);
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
return false;
}
@@ -808,6 +835,7 @@
slru_errcause = SLRU_FSYNC_FAILED;
slru_errno = errno;
close(fd);
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
return false;
}
@@ -815,10 +843,12 @@
{
slru_errcause = SLRU_CLOSE_FAILED;
slru_errno = errno;
+ TRACE_POSTGRESQL_SLRU_WRITEPAGE_PHYSICAL_DONE(false, slru_errcause, slru_errno);
