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

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

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

Robert Haas <robertmhaas(at)gmail(dot)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

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

Robert Haas <robertmhaas(at)gmail(dot)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

On Sat, Nov 14, 2009 at 6:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

--On 14. November 2009 20:22:42 -0500 Robert Haas <robertmhaas(at)gmail(dot)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

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

On Sat, Nov 14, 2009 at 4:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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

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

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

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

On Thu, Nov 26, 2009 at 4:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Nov 14, 2009 at 4:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Thu, Dec 17, 2009 at 9:15 PM, Robert Haas <robertmhaas(at)gmail(dot)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

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

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

> --- 49,63 ----
> * ----------------
> */
>
> ! #define Natts_pg_tablespace 6

Should be 5?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

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

Robert Haas <robertmhaas(at)gmail(dot)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

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

Robert Haas <robertmhaas(at)gmail(dot)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

On Mon, Jan 4, 2010 at 10:42 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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(at)alvh(dot)no-ip(dot)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

On Sun, Jan 3, 2010 at 11:19 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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

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

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.

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

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