Re: [PATCHES] Sorting writes during checkpoint
On Mon, 5 May 2008, Tom Lane wrote: It bothers me a bit that the patch forces writes to be done "all of file A in order, then all of file B in order, etc". We don't know enough about the disk layout of the files to be sure that that's good. (This might also mean that whether there is a win is going to be platform and filesystem dependent ...) I think most platform and filesystem implementations have disk location correlated enough with block order that this particular issue isn't a large one. If the writes are mainly going to one logical area (a single partition or disk array), it should be a win as long as the sorting step itself isn't introducing a delay. I am concered that in a more complicated case than pgbench, where the writes are spread across multiple arrays say, that forcing writes in order may slow things down. Example: let's say there's two tablespaces mapped to two arrays, A and B, that the data is being written to at checkpoint time. In the current case, that I/O might be AABAABABBBAB, which is going to keep both arrays busy writing. The sorted case would instead make that AABB so only one array will be active at a time. It may very well be the case that the improvement from lowering seeks on the writes to A and B is less than the loss coming from not keeping both continuously busy. I think I can simulate this by using a modified pgbench script that works against an accounts1 and accounts2 with equal frequency, where 1&2 are actually on different tablespaces on two disks. Right, that's in the ground rules for commitfests: if the submitter can respond to complaints before the fest is over, we'll reconsider the patch. The small optimization I was trying to suggest was that you just bounce this type of patch automatically to the "rejected for " section of the commitfest wiki page in cases like these. The standard practice on this sort of queue is to automatically reclassify when someone has made a pass over the patch, leaving the original source to re-open with more information. That keeps the unprocessed part of the queue always shrinking, and as long as people know that they can get it reconsidered by submitting new results it's not unfair to them. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Sorting writes during checkpoint
Greg Smith <[EMAIL PROTECTED]> writes: > On Sun, 4 May 2008, Tom Lane wrote: >> Well, I tried a pgbench test similar to that one --- on smaller hardware >> than was reported, so it was a bit smaller test case, but it should have >> given similar results. > ... If > you're not offloading to another device like that, the OS-level elevator > sorting will handle sorting for you close enough to optimally that I doubt > this will help much (and in fact may just get in the way). Yeah. It bothers me a bit that the patch forces writes to be done "all of file A in order, then all of file B in order, etc". We don't know enough about the disk layout of the files to be sure that that's good. (This might also mean that whether there is a win is going to be platform and filesystem dependent ...) >> Unless someone can volunteer to test sooner, I think we should drop this >> item from the current commitfest queue. > From the perspective of keeping the committer's plates clean, a reasonable > system for this situation might be for you to bounce this into the > rejected pile as "Returned for testing" immediately, to clearly remove it > from the main queue. A reasonable expectation there is that you might > consider it again during May if someone gets back with said testing > results before the 'fest ends. Right, that's in the ground rules for commitfests: if the submitter can respond to complaints before the fest is over, we'll reconsider the patch. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] options for RAISE statement
Hello I thing, all your comments are not problem. I'll send new version this week. Thank You Pavel Stehule 2008/5/5 Tom Lane <[EMAIL PROTECTED]>: > "Pavel Stehule" <[EMAIL PROTECTED]> writes: >> this patch adds possibility to set additional options (SQLSTATE, >> DETAIL, DETAIL_LOG and HINT) for RAISE statement, > > I looked this over briefly. A couple of comments: > > * Raising errors via hard-coded SQLSTATEs seems pretty unfriendly, > at least for cases where we are reporting built-in errors. Wouldn't > it be better to be able to raise errors using the same SQLSTATE names > that are recognized in EXCEPTION clauses? > > * If we are going to let people throw random SQLSTATEs, there had better > be a way to name those same SQLSTATEs in EXCEPTION. > > * I don't really like exposing DETAIL_LOG in this. That was a spur of > the moment addition and we might take it out again; I think it's way > premature to set it in stone by exposing it as a plpgsql feature. > > * Please avoid using errstart() directly. This is unwarranted intimacy > with elog.h's implementation and I also think it will have unpleasant > behavior if an error occurs while evaluating the RAISE arguments. > (In fact, I think a user could easily force a backend PANIC that way.) > The approved way to deal with ereport options that might not be there > is like this: > >ereport(ERROR, >( ..., > have_sqlstate ? errcode(...) : 0, > ... > > That is, you should evaluate all the options into local variables > and then do one normal ereport call. > > * // comments are against our coding conventions. > >regards, tom lane > -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Sorting writes during checkpoint
On Sun, 4 May 2008, Tom Lane wrote: Well, I tried a pgbench test similar to that one --- on smaller hardware than was reported, so it was a bit smaller test case, but it should have given similar results. My pet theory on cases where sorting will help suggests you may need a write-caching controller for this patch to be useful. I expect we'll see the biggest improvement in situations where the total amount of dirty buffers is larger than the write cache and the cache becomes blocked. If you're not offloading to another device like that, the OS-level elevator sorting will handle sorting for you close enough to optimally that I doubt this will help much (and in fact may just get in the way). Of course it's notoriously hard to get consistent numbers out of pgbench anyway, so I'd rather see some other test case ... I have some tools to run pgbench results many times and look for patterns that work fairly well for the consistency part. pgbench will dirty a very high percentage of the buffer cache by checkpoint time relative to how much work it does, which makes it close to a best case for confirming there is a potential improvement here. I think a reasonable approach is to continue trying to quantify some improvement using pgbench with an eye toward also doing DBT2 tests, which provoke similar behavior at checkpoint time. I suspect someone who already has a known good DBT2 lab setup with caching controller hardware (EDB?) might be able to do a useful test of this patch without too much trouble on their part. Unless someone can volunteer to test sooner, I think we should drop this item from the current commitfest queue. This patch took a good step forward toward being commited this round with your review, which is the important part from my perspective (as someone who would like this to be committed if it truly works). I expect that performance related patches will often take more than one commitfest to pass through. From the perspective of keeping the committer's plates clean, a reasonable system for this situation might be for you to bounce this into the rejected pile as "Returned for testing" immediately, to clearly remove it from the main queue. A reasonable expectation there is that you might consider it again during May if someone gets back with said testing results before the 'fest ends. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] temporal version of generate_series()
2008/5/5 Tom Lane <[EMAIL PROTECTED]>: > Applied with some revisions --- > > I added a timestamptz version; it didn't seem very appropriate to have > only a timestamp version. > > You can't just pick a convenient-looking OID, you must use one that > the unused_oids script reports as free. I didn't know the unused_oids script. I'll try it next time. Thanks for many notices anyway. Hitoshi Harada -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Multiline privileges in \z
Brendan Jurd wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Fri, Apr 18, 2008 at 2:37 AM, Tom Lane wrote: The function names in the patch need schema-qualification in case pg_catalog is not first in the search path. Ah, yes. I should have seen that. Thanks Tom. New version attached with schema-qualification. Committed with slight editorialization and a consequent docs change. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] options for RAISE statement
"Pavel Stehule" <[EMAIL PROTECTED]> writes: > this patch adds possibility to set additional options (SQLSTATE, > DETAIL, DETAIL_LOG and HINT) for RAISE statement, I looked this over briefly. A couple of comments: * Raising errors via hard-coded SQLSTATEs seems pretty unfriendly, at least for cases where we are reporting built-in errors. Wouldn't it be better to be able to raise errors using the same SQLSTATE names that are recognized in EXCEPTION clauses? * If we are going to let people throw random SQLSTATEs, there had better be a way to name those same SQLSTATEs in EXCEPTION. * I don't really like exposing DETAIL_LOG in this. That was a spur of the moment addition and we might take it out again; I think it's way premature to set it in stone by exposing it as a plpgsql feature. * Please avoid using errstart() directly. This is unwarranted intimacy with elog.h's implementation and I also think it will have unpleasant behavior if an error occurs while evaluating the RAISE arguments. (In fact, I think a user could easily force a backend PANIC that way.) The approved way to deal with ereport options that might not be there is like this: ereport(ERROR, ( ..., have_sqlstate ? errcode(...) : 0, ... That is, you should evaluate all the options into local variables and then do one normal ereport call. * // comments are against our coding conventions. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
David Fetter wrote: On Sun, May 04, 2008 at 07:49:25PM -0400, Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> writes: Tom Lane wrote: But it'll only be in \dT+ anyway, no? Not in this patch. Hmmm ... given that a long list of enum members would bloat the output quite a lot, I think I'd vote for putting it in \dT+. Here's one where it's only in \dT+ Yeah. Committed. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
On Sun, May 04, 2008 at 07:49:25PM -0400, Tom Lane wrote:
> Andrew Dunstan <[EMAIL PROTECTED]> writes:
> > Tom Lane wrote:
> >> But it'll only be in \dT+ anyway, no?
>
> > Not in this patch.
>
> Hmmm ... given that a long list of enum members would bloat the
> output quite a lot, I think I'd vote for putting it in \dT+.
Here's one where it's only in \dT+
Cheers,
David.
--
David Fetter <[EMAIL PROTECTED]> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: [EMAIL PROTECTED]
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Index: src/bin/psql/describe.c
===
RCS file: /projects/cvsroot/pgsql/src/bin/psql/describe.c,v
retrieving revision 1.168
diff -c -c -r1.168 describe.c
*** src/bin/psql/describe.c 2 May 2008 10:16:16 - 1.168
--- src/bin/psql/describe.c 4 May 2008 23:54:53 -
***
*** 307,315
"WHEN t.typlen < 0\n"
" THEN CAST('var' AS
pg_catalog.text)\n"
"ELSE CAST(t.typlen AS
pg_catalog.text)\n"
! " END AS \"%s\",\n",
gettext_noop("Internal name"),
! gettext_noop("Size"));
appendPQExpBuffer(&buf,
" pg_catalog.obj_description(t.oid, 'pg_type')
as \"%s\"\n",
gettext_noop("Description"));
--- 307,325
"WHEN t.typlen < 0\n"
" THEN CAST('var' AS
pg_catalog.text)\n"
"ELSE CAST(t.typlen AS
pg_catalog.text)\n"
! " END AS \"%s\",\n"
! "
pg_catalog.array_to_string(\n"
! " ARRAY(\n"
! " SELECT
e.enumlabel\n"
! " FROM
pg_catalog.pg_enum e\n"
! " WHERE e.enumtypid
= t.oid\n"
! " ORDER BY e.oid\n"
! " ),\n"
! " E'\\n'\n"
! " ) AS \"%s\",\n",
gettext_noop("Internal name"),
! gettext_noop("Size"),
! gettext_noop("Elements"));
appendPQExpBuffer(&buf,
" pg_catalog.obj_description(t.oid, 'pg_type')
as \"%s\"\n",
gettext_noop("Description"));
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
Andrew Dunstan <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> But it'll only be in \dT+ anyway, no? > Not in this patch. Hmmm ... given that a long list of enum members would bloat the output quite a lot, I think I'd vote for putting it in \dT+. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
Tom Lane wrote: Andrew Dunstan <[EMAIL PROTECTED]> writes: I notice that this patch adds an "Elements" column to the output of \dT, which will only be used by enum types. That seems rather ... cluttered. But it'll only be in \dT+ anyway, no? Not in this patch. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Sorting writes during checkpoint
Greg Smith <[EMAIL PROTECTED]> writes: > On Sun, 4 May 2008, Tom Lane wrote: >> However, I am completely unable to measure any performance improvement >> from it. Given the possible risk of out-of-memory failures, I think the >> patch should not be applied without some direct proof of performance >> benefits, and I don't see any. > Fair enough. There were some pgbench results attached to the original > patch submission that gave me a good idea how to replicate the situation > where there's some improvement. Well, I tried a pgbench test similar to that one --- on smaller hardware than was reported, so it was a bit smaller test case, but it should have given similar results. I didn't see any improvement; if anything it was a bit worse. So that's what got me concerned. Of course it's notoriously hard to get consistent numbers out of pgbench anyway, so I'd rather see some other test case ... > I expect I can take a shot at quantifying > that independantly near the end of this month if nobody else gets to it > before then (I'm stuck sorting out a number of OS level issue right now > before my testing system is online again). Was planning to take a longer > look at Greg Stark's prefetching work at that point as well. Fair enough. Unless someone can volunteer to test sooner, I think we should drop this item from the current commitfest queue. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] temporal version of generate_series()
"H.Harada" <[EMAIL PROTECTED]> writes: > Here's the sync and updated patch. > It contains "strict" in catalog as well. Applied with some revisions --- I added a timestamptz version; it didn't seem very appropriate to have only a timestamp version. You can't just pick a convenient-looking OID, you must use one that the unused_oids script reports as free. There was no check for a zero step size. There was no documentation. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Sorting writes during checkpoint
On Sun, 4 May 2008, Tom Lane wrote: However, I am completely unable to measure any performance improvement from it. Given the possible risk of out-of-memory failures, I think the patch should not be applied without some direct proof of performance benefits, and I don't see any. Fair enough. There were some pgbench results attached to the original patch submission that gave me a good idea how to replicate the situation where there's some improvement. I expect I can take a shot at quantifying that independantly near the end of this month if nobody else gets to it before then (I'm stuck sorting out a number of OS level issue right now before my testing system is online again). Was planning to take a longer look at Greg Stark's prefetching work at that point as well. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
Andrew Dunstan <[EMAIL PROTECTED]> writes: > I notice that this patch adds an "Elements" column to the output of \dT, > which will only be used by enum types. That seems rather ... cluttered. But it'll only be in \dT+ anyway, no? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
On Sun, May 04, 2008 at 06:40:51PM -0400, Andrew Dunstan wrote: > > > David Fetter wrote: >> Folks, >> >> In psql, \dT doesn't show the elements for enums. Please find >> patch vs. CVS TIP attached which fixes this per the following TODO >> item: >> >> http://archives.postgresql.org/pgsql-hackers/2008-01/msg00826.php > > I notice that this patch adds an "Elements" column to the output of > \dT, which will only be used by enum types. That seems rather ... > cluttered. Is the name too long, or did you want it rolled into one of the other columns, or...? Cheers, David. -- David Fetter <[EMAIL PROTECTED]> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: [EMAIL PROTECTED] Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
David Fetter wrote: Folks, In psql, \dT doesn't show the elements for enums. Please find patch vs. CVS TIP attached which fixes this per the following TODO item: http://archives.postgresql.org/pgsql-hackers/2008-01/msg00826.php I notice that this patch adds an "Elements" column to the output of \dT, which will only be used by enum types. That seems rather ... cluttered. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Multiline privileges in \z
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Sun, May 4, 2008 at 10:55 AM, Tom Lane wrote: > Andrew Dunstan writes: > > Wouldn't this expression: > > pg_catalog.array_to_string(c.relacl, chr(10)) > > be better expressed as > > pg_catalog.array_to_string(c.relacl, E'\n') > > +1 ... it's minor, but knowing that ASCII LF is 10 is probably not > wired into too many people's brains anymore. (Besides, some of us > remember it as octal 12, anyway...) > Fair enough. I just wanted a non-messy way of saying "1 newline, please", and I wasn't too sure whether backslash escapes were considered kosher for internal queries. But you're right, readability is important. No objections to using the escape syntax. Please note that I used chr(10) in my \du attributes patch as well. Cheers, BJ -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.7 (GNU/Linux) Comment: http://getfiregpg.org iD8DBQFIHjNA5YBsbHkuyV0RAuNSAJ0ZDLxhHaPj4CBsBCILnxHy+5Jf5ACfQHMH 4XZxczc+YEow3AFdayn9fGs= =+TSV -END PGP SIGNATURE- -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] pg_postmaster_reload_time() patch
"George Gensure" <[EMAIL PROTECTED]> writes: > The new function name is pg_conf_load_time() Applied with revisions. > I'm now using LWLocks only on the backend in order to protect the > PgReloadTime from mid copy reads. This may prove to be unnecessary, > since the code to handle HUPs seems to be executed synchronously on > the backend, but I'll let someone else tell me its safe before > removing it. The locking was not only entirely useless, but it didn't even compile, since you'd not supplied a definition for "ReloadTimeLock". I took it out. I moved the setting of PgReloadTime into ProcessConfigFile. The advantages are (1) only one place to do it, and (2) inside ProcessConfigFile, we know whether or not the reload actually "took". As committed, the definition of PgReloadTime is really "the time of the last *successful* load of postgresql.conf", which I think is more useful than "the last attempted load". I also improved the documentation a bit and added the copy step needed in restore_backend_variables(). regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Text <-> C string
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > Well ... if somebody does want to change the representation of xml > down the road, he's going to have to touch all the sites where the > code converts to and from cstring anyway, right? True. > With that in mind, please find attached my followup patch. It cleans > up another 21 sites manually copying between cstring and varlena, for > a net reduction of 115 lines of code. I applied most of this, but not the parts that were dependent on the assumption that bytea and text are the same. That is unlikely to remain true if we ever get around to putting locale/encoding information into text values. Furthermore it's a horrid type pun, because bytea can contain embedded nulls whereas cstrings can't; you were making unwarranted assumptions about whether, say, cstring_to_text_with_len would allow embedded nulls to go by. And lastly, quite a few of those changes were just plain broken, eg several places in selfuncs.c where you allowed strlen() to be applied to a "bytea converted to cstring". regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
