Thread

1. patch - per-tablespace random_page_cost/seq_page_cost

   Robert Haas <robertmhaas@gmail.com> — 2009-11-14T19:28:56Z

   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
   

2. Re: patch - per-tablespace random_page_cost/seq_page_cost

   Greg Stark <gsstark@mit.edu> — 2009-11-14T20:06:49Z

   On Sat, Nov 14, 2009 at 7:28 PM, Robert Haas <robertmhaas@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
   
   

3. Re: patch - per-tablespace random_page_cost/seq_page_cost

   Robert Haas <robertmhaas@gmail.com> — 2009-11-14T21:02:55Z

   On Sat, Nov 14, 2009 at 3:06 PM, Greg Stark <gsstark@mit.edu> wrote:
   > On Sat, Nov 14, 2009 at 7:28 PM, Robert Haas <robertmhaas@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
   
   

4. Re: patch - per-tablespace random_page_cost/seq_page_cost

   Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-14T21:58:35Z

   Robert Haas <robertmhaas@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
   
   

5. Re: patch - per-tablespace random_page_cost/seq_page_cost

   Robert Haas <robertmhaas@gmail.com> — 2009-11-14T22:56:27Z

   On Sat, Nov 14, 2009 at 4:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > Robert Haas <robertmhaas@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
   
   

6. Re: patch - per-tablespace random_page_cost/seq_page_cost

   Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-14T23:36:25Z

   Robert Haas <robertmhaas@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
   
   

7. Re: patch - per-tablespace random_page_cost/seq_page_cost

   Robert Haas <robertmhaas@gmail.com> — 2009-11-15T01:22:42Z

   On Sat, Nov 14, 2009 at 6:36 PM, Tom Lane <tgl@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
   
   

8. Re: patch - per-tablespace random_page_cost/seq_page_cost

   Bernd Helmle <mailings@oopsware.de> — 2009-11-16T09:37:54Z

   
   --On 14. November 2009 20:22:42 -0500 Robert Haas <robertmhaas@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
   
   

9. Re: patch - per-tablespace random_page_cost/seq_page_cost

   Robert Haas <robertmhaas@gmail.com> — 2009-11-16T14:26:09Z

   On Mon, Nov 16, 2009 at 4:37 AM, Bernd Helmle <mailings@oopsware.de> wrote:
   > --On 14. November 2009 20:22:42 -0500 Robert Haas <robertmhaas@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
   
   

10. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2009-11-26T21:25:15Z

    On Sat, Nov 14, 2009 at 4:58 PM, Tom Lane <tgl@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?
    
    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.
    
    ...Robert
    

11. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2009-11-27T04:16:52Z

    On Thu, Nov 26, 2009 at 4:25 PM, Robert Haas <robertmhaas@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
    

12. Re: patch - per-tablespace random_page_cost/seq_page_cost

    David Rowley <dgrowley@gmail.com> — 2009-11-29T02:54:20Z

    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
    
    
    

13. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2009-12-03T16:00:58Z

    On Sat, Nov 28, 2009 at 9:54 PM, David Rowley <dgrowley@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
    
    

14. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2009-12-18T01:57:38Z

    On Thu, Dec 3, 2009 at 11:00 AM, Robert Haas <robertmhaas@gmail.com> wrote:
    > On Sat, Nov 28, 2009 at 9:54 PM, David Rowley <dgrowley@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
    
    

15. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2009-12-18T02:15:50Z

    On Thu, Nov 26, 2009 at 4:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    > On Sat, Nov 14, 2009 at 4:58 PM, Tom Lane <tgl@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
    
    

16. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2009-12-28T07:52:46Z

    On Thu, Dec 17, 2009 at 9:15 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    > 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.
    [...]
    > 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?
    
    Hearing no thoughts, I have implemented as per the above.  PFA the
    latest version.  Any reviews, comments, feedback, etc. much
    appreciated.
    
    Thanks,
    
    ...Robert
    

17. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Jaime Casanova <jcasanov@systemguards.com.ec> — 2010-01-03T23:56:38Z

    On Mon, Dec 28, 2009 at 2:52 AM, Robert Haas <robertmhaas@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
    
    

18. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2010-01-04T03:39:40Z

    On Sun, Jan 3, 2010 at 6:56 PM, Jaime Casanova
    <jcasanov@systemguards.com.ec> wrote:
    > On Mon, Dec 28, 2009 at 2:52 AM, Robert Haas <robertmhaas@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
    
    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.
    
    ...Robert
    

19. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-01-04T04:19:28Z

    > --- 49,63 ----
    >    * ----------------
    >    */
    >   
    > ! #define Natts_pg_tablespace				6
    
    Should be 5?
    
    
    
    
    -- 
    Alvaro Herrera                                http://www.CommandPrompt.com/
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
    
    

20. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Jaime Casanova <jcasanov@systemguards.com.ec> — 2010-01-04T04:42:45Z

    On Sun, Jan 3, 2010 at 10:39 PM, Robert Haas <robertmhaas@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
    
    

21. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-01-04T15:42:32Z

    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 Herrera                                http://www.CommandPrompt.com/
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
    
    

22. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-01-04T18:39:11Z

    Robert Haas <robertmhaas@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
    
    

23. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2010-01-04T18:44:11Z

    On Mon, Jan 4, 2010 at 1:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    > Robert Haas <robertmhaas@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
    
    

24. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-01-04T18:48:45Z

    Robert Haas <robertmhaas@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
    
    

25. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2010-01-04T18:55:27Z

    On Mon, Jan 4, 2010 at 10:42 AM, Alvaro Herrera
    <alvherre@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 <alvherre@alvh.no-ip.org>
    Date:   Mon Jan 5 17:14:28 2009 +0000
    
        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
    
    

26. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2010-01-04T19:07:17Z

    On Sun, Jan 3, 2010 at 11:19 PM, Alvaro Herrera
    <alvherre@commandprompt.com> wrote:
    >
    >> --- 49,63 ----
    >>    * ----------------
    >>    */
    >>
    >> ! #define Natts_pg_tablespace                         6
    >
    > Should be 5?
    
    Yep.  I also fixed the other two bits of brain fade that you pointed
    out to me via private email.  Updated patch attached.
    
    ...Robert
    

27. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2010-01-04T19:17:19Z

    On Mon, Jan 4, 2010 at 1:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    > Robert Haas <robertmhaas@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
    
    

28. Re: patch - per-tablespace random_page_cost/seq_page_cost

    John Naylor <jcnaylor@gmail.com> — 2010-01-05T00:30:02Z

    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.
    

29. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2010-01-05T15:17:05Z

    On Mon, Jan 4, 2010 at 1:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    > Robert Haas <robertmhaas@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
    
    

30. Re: patch - per-tablespace random_page_cost/seq_page_cost

    Robert Haas <robertmhaas@gmail.com> — 2010-01-06T16:07:04Z

    On Tue, Jan 5, 2010 at 10:17 AM, Robert Haas <robertmhaas@gmail.com> wrote:
    > On Mon, Jan 4, 2010 at 1:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    >> Robert Haas <robertmhaas@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