Re: [HACKERS] patch - per-tablespace random_page_cost/seq_page_cost
On Tue, Jan 5, 2010 at 10:17 AM, Robert Haas robertmh...@gmail.com wrote:
On Mon, Jan 4, 2010 at 1:48 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Robert Haas robertmh...@gmail.com writes:
My only objection to that is that if we're going to add attoptions
also, I'd like to get this committed first before I start working on
that, and we're running short on time. If you can commit his patch in
the next day or two, then I am fine with rebasing mine afterwards, but
if it needs more work than that then I would prefer to commit mine so
I can move on. Is that reasonable?
Fair enough --- if I can't get it done today I will let you know and
hold off.
OK, so since you got this done, I'm going to go ahead and rebase
commit mine today, after a final read-through or two, unless you or
anyone else wants to insert some last-minute objections?
I committed this, but then in looking some things over further today,
I realized that I seem to have done something stupid - namely, not
adding a varlena header to TableSpaceOpts. I believe that the
attached patch is needed to fix the problem.
(I am not quite sure why we are using bytea here since AFAICS we don't
actually store parsed reloptions structures in any kind of persistent
storage, but clearly overwriting the first four bytes of
random_page_cost with a varlena header is no good.)
...Robert
diff --git a/src/include/commands/tablespace.h b/src/include/commands/tablespace.h
index b973450..cf005ee 100644
--- a/src/include/commands/tablespace.h
+++ b/src/include/commands/tablespace.h
@@ -34,6 +34,7 @@ typedef struct xl_tblspc_drop_rec
typedef struct TableSpaceOpts
{
+ int32 vl_len_; /* varlena header (do not touch directly!) */
float8 random_page_cost;
float8 seq_page_cost;
} TableSpaceOpts;
--
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 - per-tablespace random_page_cost/seq_page_cost
On Mon, Jan 4, 2010 at 1:48 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: My only objection to that is that if we're going to add attoptions also, I'd like to get this committed first before I start working on that, and we're running short on time. If you can commit his patch in the next day or two, then I am fine with rebasing mine afterwards, but if it needs more work than that then I would prefer to commit mine so I can move on. Is that reasonable? Fair enough --- if I can't get it done today I will let you know and hold off. OK, so since you got this done, I'm going to go ahead and rebase commit mine today, after a final read-through or two, unless you or anyone else wants to insert some last-minute objections? ...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 - per-tablespace random_page_cost/seq_page_cost
Robert Haas escribió: Hmm, I see this needs to be rebased over Tom's latest changes, but the conflict I got was in syscache.h, rather than syscache.c. Not sure if that's what you were going for or if there's another issue. Updated patch attached. FWIW I think the reloptions code in this patch is sane enough. The fact that it was this easily written means that the API for reloptions was reasonably chosen, thanks :-) Hmm, it seems we're missing a need_initialization = false at the bottom of initialize_reloptions ... I'm wondering what happened to that?? -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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 - per-tablespace random_page_cost/seq_page_cost
Robert Haas robertmh...@gmail.com writes: Hmm, I see this needs to be rebased over Tom's latest changes, but the conflict I got was in syscache.h, rather than syscache.c. Not sure if that's what you were going for or if there's another issue. Updated patch attached. I'm planning to go look at Naylor's bki refactoring patch now. Assuming there isn't any showstopper problem with that, do you object to it getting committed first? Either order is going to create a merge problem, but it seems like we'd be best off to get Naylor's patch in so people can resync affected patches before the January commitfest starts. regards, tom lane -- 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 - per-tablespace random_page_cost/seq_page_cost
On Mon, Jan 4, 2010 at 1:39 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Hmm, I see this needs to be rebased over Tom's latest changes, but the conflict I got was in syscache.h, rather than syscache.c. Not sure if that's what you were going for or if there's another issue. Updated patch attached. I'm planning to go look at Naylor's bki refactoring patch now. Assuming there isn't any showstopper problem with that, do you object to it getting committed first? Either order is going to create a merge problem, but it seems like we'd be best off to get Naylor's patch in so people can resync affected patches before the January commitfest starts. My only objection to that is that if we're going to add attoptions also, I'd like to get this committed first before I start working on that, and we're running short on time. If you can commit his patch in the next day or two, then I am fine with rebasing mine afterwards, but if it needs more work than that then I would prefer to commit mine so I can move on. Is that reasonable? ...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 - per-tablespace random_page_cost/seq_page_cost
Robert Haas robertmh...@gmail.com writes: My only objection to that is that if we're going to add attoptions also, I'd like to get this committed first before I start working on that, and we're running short on time. If you can commit his patch in the next day or two, then I am fine with rebasing mine afterwards, but if it needs more work than that then I would prefer to commit mine so I can move on. Is that reasonable? Fair enough --- if I can't get it done today I will let you know and hold off. regards, tom lane -- 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 - per-tablespace random_page_cost/seq_page_cost
On Mon, Jan 4, 2010 at 10:42 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: Robert Haas escribió: Hmm, I see this needs to be rebased over Tom's latest changes, but the conflict I got was in syscache.h, rather than syscache.c. Not sure if that's what you were going for or if there's another issue. Updated patch attached. FWIW I think the reloptions code in this patch is sane enough. The fact that it was this easily written means that the API for reloptions was reasonably chosen, thanks :-) :-) Actually, there are some things about it that I'm not entirely happy with, but I haven't brought them up because I don't have a clear idea what I think we should do about them. The special-case hack to handle the oids option is one of them another, possibly related, is that I wish we could decouple the options-validation logic from the backend storage representation. But those are issues for a future thread. I do think it's pretty well-done overall. Hmm, it seems we're missing a need_initialization = false at the bottom of initialize_reloptions ... I'm wondering what happened to that?? It appears that it has never been there. $ git log -Sneed_initialization master src/backend/access/common/reloptions.c commit f35e4442a6c9893e72fe870d9e1756262d542027 Author: Alvaro Herrera alvhe...@alvh.no-ip.org Date: Mon Jan 5 17:14:28 2009 + Change the reloptions machinery to use a table-based parser, and provide a more complete framework for writing custom option processing routines by user-defined access methods. Catalog version bumped due to the general API changes, which are going to affect user-defined amoptions routines. That was the original patch that added need_initialization, and it didn't add that line. ...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 - per-tablespace random_page_cost/seq_page_cost
On Mon, Jan 4, 2010 at 1:48 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: My only objection to that is that if we're going to add attoptions also, I'd like to get this committed first before I start working on that, and we're running short on time. If you can commit his patch in the next day or two, then I am fine with rebasing mine afterwards, but if it needs more work than that then I would prefer to commit mine so I can move on. Is that reasonable? Fair enough --- if I can't get it done today I will let you know and hold off. OK. I just took a really fast look at that the bki patch and it looks pretty nice, so I hope you're able to get it in. Of course, I'm biased because it's based on earlier work of my own, but biased != wrong. :-) A lot more work will need to be done to escape the insanity that is our current method of handling system catalogs, but this seems like a good step in the right direction. I also observe that it applies cleanly over my current spcoptions branch, so the merge conflicts might be a non-issue. ...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 - per-tablespace random_page_cost/seq_page_cost
Tom, It seems I introduced a couple errors in src/tools/msvc/clean.bat in the bki patch. I'm attaching a cumulative fix. I can resend the complete updated patch, if you like... Sorry! :-) John I'm planning to go look at Naylor's bki refactoring patch now. Assuming there isn't any showstopper problem with that, do you object to it getting committed first? Either order is going to create a merge problem, but it seems like we'd be best off to get Naylor's patch in so people can resync affected patches before the January commitfest starts. diff --git a/src/tools/msvc/clean.bat b/src/tools/msvc/clean.bat index 8cce31e..1d3ea65 100755 *** a/src/tools/msvc/clean.bat --- b/src/tools/msvc/clean.bat *** REM Delete files created with GenerateFi *** 20,35 if exist src\include\pg_config.h del /q src\include\pg_config.h if exist src\include\pg_config_os.h del /q src\include\pg_config_os.h if %DIST%==1 if exist src\backend\parser\gram.h del /q src\backend\parser\gram.h ! if exist src\include\utils\fmgroids.h del /q src\include\utils\fmgroids.h ! if exist src\include\catalog\schemapg.h del /q src\include\catalog\schemapg.h if exist src\include\utils\probes.h del /q src\include\utils\probes.h - if %DIST%==1 if exist src\backend\utils\fmgroids.h del /q src\backend\utils\fmgroids.h if %DIST%==1 if exist src\backend\utils\fmgrtab.c del /q src\backend\utils\fmgrtab.c if %DIST%==1 if exist src\backend\catalog\postgres.bki del /q src\backend\catalog\postgres.bki if %DIST%==1 if exist src\backend\catalog\postgres.description del /q src\backend\catalog\postgres.description if %DIST%==1 if exist src\backend\catalog\postgres.shdescription del /q src\backend\catalog\postgres.shdescription - if %DIST%==1 if exist src\backend\catalog\schemapg.h del /q src\backend\catalog\schemapg.h if %DIST%==1 if exist src\backend\parser\scan.c del /q src\backend\parser\scan.c if %DIST%==1 if exist src\backend\parser\gram.c del /q src\backend\parser\gram.c if %DIST%==1 if exist src\backend\bootstrap\bootscanner.c del /q src\backend\bootstrap\bootscanner.c --- 20,33 if exist src\include\pg_config.h del /q src\include\pg_config.h if exist src\include\pg_config_os.h del /q src\include\pg_config_os.h if %DIST%==1 if exist src\backend\parser\gram.h del /q src\backend\parser\gram.h ! if %DIST%==1 if exist src\backend\utils\fmgroids.h del /q src\backend\utils\fmgroids.h ! if %DIST%==1 if exist src\backend\catalog\schemapg.h del /q src\backend\catalog\schemapg.h if exist src\include\utils\probes.h del /q src\include\utils\probes.h if %DIST%==1 if exist src\backend\utils\fmgrtab.c del /q src\backend\utils\fmgrtab.c if %DIST%==1 if exist src\backend\catalog\postgres.bki del /q src\backend\catalog\postgres.bki if %DIST%==1 if exist src\backend\catalog\postgres.description del /q src\backend\catalog\postgres.description if %DIST%==1 if exist src\backend\catalog\postgres.shdescription del /q src\backend\catalog\postgres.shdescription if %DIST%==1 if exist src\backend\parser\scan.c del /q src\backend\parser\scan.c if %DIST%==1 if exist src\backend\parser\gram.c del /q src\backend\parser\gram.c if %DIST%==1 if exist src\backend\bootstrap\bootscanner.c del /q src\backend\bootstrap\bootscanner.c -- 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 - per-tablespace random_page_cost/seq_page_cost
On Mon, Dec 28, 2009 at 2:52 AM, Robert Haas robertmh...@gmail.com wrote: Hearing no thoughts, I have implemented as per the above. PFA the latest version. Any reviews, comments, feedback, etc. much appreciated. btw, you need to change STATRELATT, for STATRELATTINH, in syscache.c -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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 - per-tablespace random_page_cost/seq_page_cost
--- 49,63 * */ ! #define Natts_pg_tablespace 6 Should be 5? -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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 - per-tablespace random_page_cost/seq_page_cost
On Sun, Jan 3, 2010 at 10:39 PM, Robert Haas robertmh...@gmail.com wrote: in syscache.c Hmm, I see this needs to be rebased over Tom's latest changes, but the conflict I got was in syscache.h, rather than syscache.c. Not sure if that's what you were going for or if there's another issue. Updated patch attached. ah! yeah! it has been a long holiday ;) -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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 - per-tablespace random_page_cost/seq_page_cost
On Thu, Dec 3, 2009 at 11:00 AM, Robert Haas robertmh...@gmail.com wrote: On Sat, Nov 28, 2009 at 9:54 PM, David Rowley dgrow...@gmail.com wrote: Robert Haas Wrote: Hmm. I'm not able to reliably detect a performance difference between unpatched CVS HEAD (er... git master branch) and same with spcoptions- v2.patch applied. I figured that if there were going to be an impact, it would be most likely to manifest itself in a query that touches lots and lots of tables but does very little actual work. So I used the attached script to create 200 empty tables, 100 in the default tablespace and 100 in tablespace dork (also known as, why I am working on this at 11 PM on Thanksgiving). Then I did: SELECT * FROM a1, a2, a3, ..., a100; (I've not read the patch, but I've just read the thread) If you're just benchmarking the planner times to see if the extra lookups are affecting the planning times, would it not be better to benchmark EXPLAIN SELECT * FROM a1, a2, a3, ..., a100; ? Otherwise any small changes might be drowned out in the execution time. Scanning 100 relations even if they are empty could account for quite a bit of that time, right? Possibly, but even if I can measure a difference doing it that way, it's not clear that it matters. It's fairly certain that there will be a performance degradation if we measure carefully enough, but if that difference is imperceptible in real-world scanerios, then it's not worth worrying about. Still, I probably will test it just to see. I did some fairly careful benchmarking of EXPLAIN SELECT * FROM a1, a2, a3, ..., a100. I explained this query 100 times via DBD::Pg and used time to measure how long the script took to run. I ran the script three times. And the result is... the unpatched version came out 1.7% SLOWER than the patched version. This seems difficult to take seriously, since it can't possibly be faster to do a syscache lookup and parse an array than it is to fetch a constant from a known memory address, but that's what I got. At any rate, it seems pretty clear that it's not hurting much. ...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 - per-tablespace random_page_cost/seq_page_cost
On Thu, Nov 26, 2009 at 4:25 PM, Robert Haas robertmh...@gmail.com wrote: On Sat, Nov 14, 2009 at 4:58 PM, Tom Lane t...@sss.pgh.pa.us wrote: I don't think there's even a solid consensus right now on which GUCs people would want to set at the tablespace level. This seems like an important point that we need to nail down. The original motivation for this patch was based on seq_page_cost and random_page_cost, to cover the case where, for example, one tablespace is on an SSD and another tablespace is on a RAID array. Greg Stark proposed adding effective_io_concurrency, and that makes plenty of sense to me, but I'm sort of disinclined to attempt to implement that as part of this patch because I have no familiarity with that part of the code and no hardware that I can use to test either the current behavior or the modified behavior. Since I'm recoding this to use the reloptions mechanism, a patch to add support for that should be pretty easy to write as a follow-on patch once this goes in. Any other suggestions? Going once... going twice... since no one has suggested anything or spoken against the proposal above, I'm just going to implement seq_page_cost and random_page_cost for now. Current version of patch is attached. I've revised it to use the reloptions stuff, but I don't think it's committable as-is because it currently thinks that extracting options from a pg_tablespace tuple is a cheap operation, which was true in the non-reloptions-based implementation but is less true now. At least, some benchmarking needs to be done to figure out whether and to what extent this is an issue. Per the email that I just sent a few minutes ago, there doesn't appear to be a performance impact in doing this even in a relatively stupid way - every call that requires seq_page_cost and/or random_page_cost results in a syscache lookup and then uses the relcache machinery to parse the returned array. That leaves the question of what the most elegant design is here. Tom suggested upthread that we should tag every RelOptInfo - and, presumably, IndexOptInfo, though it wasn't discussed - with this information. I don't however much like the idea of adding identically named members in both places. Should the number of options expand in the future, this will become silly very quickly. One option is to define a struct with seq_page_cost and random_page_cost that is then included in RelOptInfo and IndexOptInfo. It would seem to make sense to make the struct, rather than a pointer to the struct, the member, because it makes the copyfuncs/equalfuncs stuff easier to handle, and there's not really any benefit in incurring more palloc overhead. However, I'm sort of inclined to go ahead and invent a mini-cache for tablespaces. It avoids the (apparently insignificant) overhead of reparsing the array multiple times, but it also avoids bloating RelOptInfo and IndexOptInfo with more members than really necessary. It seems like a good idea to add one member to those structures anyway, for reltablespace, but copying all the values into every one we create just seems silly. Admittedly there are only two values right now, but again we may want to add more someday, and caching at the tablespace level just seems like the right way to do it. Thoughts? ...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 - per-tablespace random_page_cost/seq_page_cost
On Sat, Nov 28, 2009 at 9:54 PM, David Rowley dgrow...@gmail.com wrote: Robert Haas Wrote: Hmm. I'm not able to reliably detect a performance difference between unpatched CVS HEAD (er... git master branch) and same with spcoptions- v2.patch applied. I figured that if there were going to be an impact, it would be most likely to manifest itself in a query that touches lots and lots of tables but does very little actual work. So I used the attached script to create 200 empty tables, 100 in the default tablespace and 100 in tablespace dork (also known as, why I am working on this at 11 PM on Thanksgiving). Then I did: SELECT * FROM a1, a2, a3, ..., a100; (I've not read the patch, but I've just read the thread) If you're just benchmarking the planner times to see if the extra lookups are affecting the planning times, would it not be better to benchmark EXPLAIN SELECT * FROM a1, a2, a3, ..., a100; ? Otherwise any small changes might be drowned out in the execution time. Scanning 100 relations even if they are empty could account for quite a bit of that time, right? Possibly, but even if I can measure a difference doing it that way, it's not clear that it matters. It's fairly certain that there will be a performance degradation if we measure carefully enough, but if that difference is imperceptible in real-world scanerios, then it's not worth worrying about. Still, I probably will test it just to see. ...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 - per-tablespace random_page_cost/seq_page_cost
Robert Haas Wrote: Hmm. I'm not able to reliably detect a performance difference between unpatched CVS HEAD (er... git master branch) and same with spcoptions- v2.patch applied. I figured that if there were going to be an impact, it would be most likely to manifest itself in a query that touches lots and lots of tables but does very little actual work. So I used the attached script to create 200 empty tables, 100 in the default tablespace and 100 in tablespace dork (also known as, why I am working on this at 11 PM on Thanksgiving). Then I did: SELECT * FROM a1, a2, a3, ..., a100; (I've not read the patch, but I've just read the thread) If you're just benchmarking the planner times to see if the extra lookups are affecting the planning times, would it not be better to benchmark EXPLAIN SELECT * FROM a1, a2, a3, ..., a100; ? Otherwise any small changes might be drowned out in the execution time. Scanning 100 relations even if they are empty could account for quite a bit of that time, right? David -- 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 - per-tablespace random_page_cost/seq_page_cost
On Thu, Nov 26, 2009 at 4:25 PM, Robert Haas robertmh...@gmail.com wrote: Current version of patch is attached. I've revised it to use the reloptions stuff, but I don't think it's committable as-is because it currently thinks that extracting options from a pg_tablespace tuple is a cheap operation, which was true in the non-reloptions-based implementation but is less true now. At least, some benchmarking needs to be done to figure out whether and to what extent this is an issue. Hmm. I'm not able to reliably detect a performance difference between unpatched CVS HEAD (er... git master branch) and same with spcoptions-v2.patch applied. I figured that if there were going to be an impact, it would be most likely to manifest itself in a query that touches lots and lots of tables but does very little actual work. So I used the attached script to create 200 empty tables, 100 in the default tablespace and 100 in tablespace dork (also known as, why I am working on this at 11 PM on Thanksgiving). Then I did: SELECT * FROM a1, a2, a3, ..., a100; ...and likewise for the bn. I tried this on an unpatched install and also with the patch applied, with and without options set on tablespace dork. I tried it a couple of times and the times were pretty consistent on any given run, but bounced around enough between runs that I can't say with any confidence that this patch makes any difference one way or the other. So it seems as if there is little reason to worry about caching, as Tom suspected, unless someone sees a flaw in my testing methodology. It might matter more in the future, if we have a larger number of tablespace options, but we could always add a cache then if need be. ...Robert many-tables.sql 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 - per-tablespace random_page_cost/seq_page_cost
--On 14. November 2009 20:22:42 -0500 Robert Haas robertmh...@gmail.com wrote: I will take another crack at it. ...Robert I take this that you are going to provide a new patch version? -- 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 - per-tablespace random_page_cost/seq_page_cost
On Mon, Nov 16, 2009 at 4:37 AM, Bernd Helmle maili...@oopsware.de wrote: --On 14. November 2009 20:22:42 -0500 Robert Haas robertmh...@gmail.com wrote: I will take another crack at it. ...Robert I take this that you are going to provide a new patch version? Yes. I'm not sure whether or not it will be in time for this CF, however. ...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 - per-tablespace random_page_cost/seq_page_cost
On Sat, Nov 14, 2009 at 6:36 PM, Tom Lane t...@sss.pgh.pa.us wrote: I'm not really convinced of that, but even if we do, so what? It's not that much code to have an extra cache watching the syscache traffic. There's an example in parse_oper.c of a specialized cache that's about as complicated as this would be. It's about 150 lines including copious comments. We didn't even bother to split it out into its own source file. Well, if it's that simple maybe it's not too bad. I'll take a look at that one. With hardwired columns, a regular catcache is all we need. But the reloptions stuff is designed to populate a struct, and once we populate that struct we have to have someplace to hang it - or I guess maybe we could reparse it on every call to cost_seqscan(), cost_index(), genericcostestimate(), etc, but that doesn't seem like a great idea. Well, no, we would not do it that way. I would imagine instead that plancat.c would be responsible for attaching appropriate cost values to each RelOptInfo struct, so it'd be more like one lookup per referenced table per query. It's possible that a cache would be useful even at that load level, but I'm not convinced. I'm not sure exactly what you mean by the last sentence, but my current design attaches the tablespace OID to RelOptInfo (for baserels only, of course) and IndexOptInfo, and the costing functions trigger the actual lookup of the page costs. I guess that might be slightly inferior to actually attaching the actualized values to the RelOptInfo, since each possible index-path needs the values for both the index and the underlying table. I will take another crack at it. ...Robert -- 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 - per-tablespace random_page_cost/seq_page_cost
Well, I was regretting missing the deadline for this CommitFest and then realized today was only the 14th, so I finished this up while the kids were napping. I ended up not reusing the reloptions.c code. It looks like a lot of extra complexity for no obvious benefit, considering that there is no equivalent of AMs for tablespaces and therefore no need to support AM-specific options. I did reuse the reloptions syntax, and I think the internal representation could always be redone later, if we find that there's a use case for something more complicated. ...Robert *** a/doc/src/sgml/config.sgml --- b/doc/src/sgml/config.sgml *** *** 1935,1940 archive_command = 'copy %p C:\\server\\archivedir\\%f' # Windows --- 1935,1943 para Sets the planner's estimate of the cost of a disk page fetch that is part of a series of sequential fetches. The default is 1.0. + This value can be overriden for a particular tablespace by setting + the tablespace parameter of the same name + (see xref linkend=sql-altertablespace). /para /listitem /varlistentry *** *** 1948,1953 archive_command = 'copy %p C:\\server\\archivedir\\%f' # Windows --- 1951,1962 para Sets the planner's estimate of the cost of a non-sequentially-fetched disk page. The default is 4.0. + This value can be overriden for a particular tablespace by setting + the tablespace parameter of the same name + (see xref linkend=sql-altertablespace). +/para + + para Reducing this value relative to varnameseq_page_cost/ will cause the system to prefer index scans; raising it will make index scans look relatively more expensive. You can raise *** a/doc/src/sgml/ref/alter_tablespace.sgml --- b/doc/src/sgml/ref/alter_tablespace.sgml *** *** 23,28 PostgreSQL documentation --- 23,30 synopsis ALTER TABLESPACE replaceablename/replaceable RENAME TO replaceablenew_name/replaceable ALTER TABLESPACE replaceablename/replaceable OWNER TO replaceablenew_owner/replaceable + ALTER TABLESPACE replaceablename/replaceable SET ( replaceable class=PARAMETERtablespace_option/replaceable = replaceable class=PARAMETERvalue/replaceable [, ... ] ) + ALTER TABLESPACE replaceablename/replaceable RESET ( replaceable class=PARAMETERtablespace_option/replaceable [, ... ] ) /synopsis /refsynopsisdiv *** *** 74,79 ALTER TABLESPACE replaceablename/replaceable OWNER TO replaceablenew_owner --- 76,99 /para /listitem /varlistentry + +varlistentry + termreplaceable class=parametertablespace_parameter/replaceable/term + listitem + para + A tablespace parameter to be set or reset. Currently, the only + available parameters are varnameseq_page_cost/ and + varnamerandom_page_cost/. Setting either value for a particular + tablespace will override the planner's usual estimate of the cost of + reading pages from tables in that tablespace, as established by + the configuration parameters of the same name (see + xref linkend=guc-seq-page-cost, + xref linkend=guc-random-page-cost). This may be useful if one + tablespace is located on a disk which is faster or slower than the + remainder of the I/O subsystem. + /para + /listitem +/varlistentry /variablelist /refsect1 *** a/src/backend/catalog/aclchk.c --- b/src/backend/catalog/aclchk.c *** *** 2621,2638 ExecGrant_Tablespace(InternalGrant *istmt) int nnewmembers; Oid *oldmembers; Oid *newmembers; - ScanKeyData entry[1]; - SysScanDesc scan; HeapTuple tuple; ! /* There's no syscache for pg_tablespace, so must look the hard way */ ! ScanKeyInit(entry[0], ! ObjectIdAttributeNumber, ! BTEqualStrategyNumber, F_OIDEQ, ! ObjectIdGetDatum(tblId)); ! scan = systable_beginscan(relation, TablespaceOidIndexId, true, ! SnapshotNow, 1, entry); ! tuple = systable_getnext(scan); if (!HeapTupleIsValid(tuple)) elog(ERROR, cache lookup failed for tablespace %u, tblId); --- 2621,2631 int nnewmembers; Oid *oldmembers; Oid *newmembers; HeapTuple tuple; ! /* Search syscache for pg_tablespace */ ! tuple = SearchSysCache(TABLESPACEOID, ObjectIdGetDatum(tblId), ! 0, 0, 0); if (!HeapTupleIsValid(tuple)) elog(ERROR, cache lookup failed for tablespace %u, tblId); *** *** 2703,2709 ExecGrant_Tablespace(InternalGrant *istmt) noldmembers, oldmembers, nnewmembers, newmembers); ! systable_endscan(scan); pfree(new_acl); --- 2696,2702 noldmembers, oldmembers, nnewmembers, newmembers); ! ReleaseSysCache(tuple); pfree(new_acl); *** *** 3443,3451
Re: [HACKERS] patch - per-tablespace random_page_cost/seq_page_cost
On Sat, Nov 14, 2009 at 7:28 PM, Robert Haas robertmh...@gmail.com wrote: I ended up not reusing the reloptions.c code. It looks like a lot of extra complexity for no obvious benefit, considering that there is no equivalent of AMs for tablespaces and therefore no need to support AM-specific options. I did reuse the reloptions syntax, and I think the internal representation could always be redone later, if we find that there's a use case for something more complicated. a) effective_io_concurrency really deserves to be in the list as well. b) I thought Tom came down pretty stridently against any data model which hard codes a specific list of supported options. I can't remember exactly what level of flexibility he wanted but I think doesn't require catalog changes to add a new option might have been it. I agree that having everything smashed to text is a bit kludgy though. I'm not sure we have the tools to do much better though. -- greg -- 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 - per-tablespace random_page_cost/seq_page_cost
On Sat, Nov 14, 2009 at 3:06 PM, Greg Stark gsst...@mit.edu wrote: On Sat, Nov 14, 2009 at 7:28 PM, Robert Haas robertmh...@gmail.com wrote: I ended up not reusing the reloptions.c code. It looks like a lot of extra complexity for no obvious benefit, considering that there is no equivalent of AMs for tablespaces and therefore no need to support AM-specific options. I did reuse the reloptions syntax, and I think the internal representation could always be redone later, if we find that there's a use case for something more complicated. a) effective_io_concurrency really deserves to be in the list as well. b) I thought Tom came down pretty stridently against any data model which hard codes a specific list of supported options. I can't remember exactly what level of flexibility he wanted but I think doesn't require catalog changes to add a new option might have been it. I agree that having everything smashed to text is a bit kludgy though. I'm not sure we have the tools to do much better though. I'm hoping Tom will reconsider, or at least flesh out his thinking. What the reloptions code does is create a general framework for handling options. Everything gets smashed down to text[], and then when we actually need to use the reloptions we parse them into a C struct appropriate to the underlying object type. This is really the only feasible design, because pg_class contains multiple different types of objects - in particular, tables and indices - and indices in turn come in multiple types, depending on the AM. So the exact options that are legal depend on the the type of object, and for indices the AM, and we populate a *different* C struct depending on the situation. pg_tablespace, on the other hand, only contains one type of object: a tablespace. So, if we stored the options as text[], we'd parse them out into a C struct just as we do for pg_class, but unlike the pg_class case, it would always be the *same* C struct. In other words, we CAN'T use dedicated columns for pg_class because we can't know in advance precisely what columns will be needed - it depends on what AMs someone chooses to load up. For pg_tablespace, we know exactly what columns will be needed, and the answer is exactly those options that we choose to support, because tablespaces are not extensible. That's my thinking, anyway... YMMV. ...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 - per-tablespace random_page_cost/seq_page_cost
Robert Haas robertmh...@gmail.com writes: pg_tablespace, on the other hand, only contains one type of object: a tablespace. So, if we stored the options as text[], we'd parse them out into a C struct just as we do for pg_class, but unlike the pg_class case, it would always be the *same* C struct. The same, until it's different. There is no reason at all to suppose that the set of options people will want to apply to a tablespace will remain constant over time --- in fact, I don't think there's even a solid consensus right now on which GUCs people would want to set at the tablespace level. I don't believe it is wise to hardwire this into the catalog schema. Yes, it would look marginally nicer from a theoretical standpoint, but we'd be forever having to revise the schema, plus a lot of downstream code (pg_dump for example); which is not only significant work but absolutely prevents making any adjustments except at major version boundaries. And I don't see any concrete benefit that we get out of a hardwired schema for these things. It's not like we care about optimizing searches for tablespaces having a particular option setting, for example. regards, tom lane -- 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 - per-tablespace random_page_cost/seq_page_cost
On Sat, Nov 14, 2009 at 4:58 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: pg_tablespace, on the other hand, only contains one type of object: a tablespace. So, if we stored the options as text[], we'd parse them out into a C struct just as we do for pg_class, but unlike the pg_class case, it would always be the *same* C struct. The same, until it's different. There is no reason at all to suppose that the set of options people will want to apply to a tablespace will remain constant over time --- in fact, I don't think there's even a solid consensus right now on which GUCs people would want to set at the tablespace level. I don't believe it is wise to hardwire this into the catalog schema. Yes, it would look marginally nicer from a theoretical standpoint, but we'd be forever having to revise the schema, plus a lot of downstream code (pg_dump for example); which is not only significant work but absolutely prevents making any adjustments except at major version boundaries. And I don't see any concrete benefit that we get out of a hardwired schema for these things. It's not like we care about optimizing searches for tablespaces having a particular option setting, for example. I can tell I've lost this argument, but I still don't get it. Why do we care if we have to change the schema? It's not a lot of work, and the number of times we would likely bump catversion for new pg_tablespace options seems unlikely to be significant in the grand scheme of things. I don't think there are very many parameters that make sense to set per-tablespace. As for major version boundaries, it seems almost unimaginable that we would backpatch code to add a new tablespace option whether the schema permits it or not. Can you clarify the nature of your concern here? What I'm concerned about with text[] is that I *think* it's going to force us to invent an analog of the relcache for tablespaces. With hardwired columns, a regular catcache is all we need. But the reloptions stuff is designed to populate a struct, and once we populate that struct we have to have someplace to hang it - or I guess maybe we could reparse it on every call to cost_seqscan(), cost_index(), genericcostestimate(), etc, but that doesn't seem like a great idea. So it seems like we'll need another caching layer sitting over the catcache. If we already had such a beast it would be reasonable to add this in, but I would assume that we wouldn't want to add such a thing without a fairly clear use case that I'm not sure we have. Maybe you see it differently? Or do you have some idea for a simpler way to set this up? ...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 - per-tablespace random_page_cost/seq_page_cost
Robert Haas robertmh...@gmail.com writes: I can tell I've lost this argument, but I still don't get it. Why do we care if we have to change the schema? It's not a lot of work, Try doing it a few times. Don't forget to keep psql and pg_dump apprised of which PG versions contain which columns. Not to mention other tools such as pgAdmin that might like to show these settings. It gets old pretty fast. What I'm concerned about with text[] is that I *think* it's going to force us to invent an analog of the relcache for tablespaces. I'm not really convinced of that, but even if we do, so what? It's not that much code to have an extra cache watching the syscache traffic. There's an example in parse_oper.c of a specialized cache that's about as complicated as this would be. It's about 150 lines including copious comments. We didn't even bother to split it out into its own source file. With hardwired columns, a regular catcache is all we need. But the reloptions stuff is designed to populate a struct, and once we populate that struct we have to have someplace to hang it - or I guess maybe we could reparse it on every call to cost_seqscan(), cost_index(), genericcostestimate(), etc, but that doesn't seem like a great idea. Well, no, we would not do it that way. I would imagine instead that plancat.c would be responsible for attaching appropriate cost values to each RelOptInfo struct, so it'd be more like one lookup per referenced table per query. It's possible that a cache would be useful even at that load level, but I'm not convinced. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
