Re: [HACKERS] Re: patch: fix performance problems with repated decomprimation of varlena values in plpgsql

[Heikki Linnakangas]

On 25.01.2011 06:29, Pavel Stehule wrote:

2011/1/25 Noah Misch:

On Sat, Jan 22, 2011 at 11:32:02AM +0100, Pavel Stehule wrote:

because I am not sure so any complex solution can be done to deadline
for 9.1, I created a patch that is based on Tom ideas - just
explicitly detoast function parameters.


I can confirm that, for your original test case, this yields performance
comparable to that of your original patch.


I know it :(. I am thinking, so detoasting on usage is better, but I
am don't know more about Tom or Rober's plans.


Detoasting on first usage, ie. exec_eval_datum(), seems the best to me. 
Compared to detoasting on assignment, it avoids the performance 
regression if the value is never used, and I don't think checking if the 
value is toasted at every exec_eval_datum() call adds too much overhead.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: patch: fix performance problems with repated decomprimation of varlena values in plpgsql

[Pavel Stehule]

Hello

2011/1/25 Noah Misch :
> On Sat, Jan 22, 2011 at 11:32:02AM +0100, Pavel Stehule wrote:
>> because I am not sure so any complex solution can be done to deadline
>> for 9.1, I created a patch that is based on Tom ideas - just
>> explicitly detoast function parameters.
>
> I can confirm that, for your original test case, this yields performance
> comparable to that of your original patch.

I know it :(. I am thinking, so detoasting on usage is better, but I
am don't know more about Tom or Rober's plans.

>
> This patch hooks into plpgsql_exec_function, detoasting only the function
> arguments.  Therefore, it doesn't help in a test case like the one I posted in
> my original review.  That test case initialized a variable using SELECT INTO,
> then used the variable in a loop.  Is there any benefit to doing this in
> plpgsql_exec_function, versus exec_assign_value (Tom's suggestion), which 
> would
> presumably help the other test case also?

I can explicitly detosting on assign stmt too. It's 6 lines more.

Regards

Pavel

>
> As we've discussed, unlike the original patch, this yields similarly grand
> performance regressions on functions that receive toasted arguments and never
> use them.  Who is prepared to speculate that this will help more people than 
> it
> will hurt?  This patch is easier on -hackers than the original, but it seems
> much more likely to create measurable performance regressions in the field.
> It's clear the committers prefer it this way, but I remain skeptical.
>
> nm
>

-- 
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] ALTER TYPE 3: add facility to identify further no-work cases

[Noah Misch]

On Mon, Jan 24, 2011 at 07:18:47PM -0500, Robert Haas wrote:
> On Sun, Jan 9, 2011 at 5:03 PM, Noah Misch  wrote:
> > Here I add the notion of an "exemptor function", a property of a cast that
> > determines when calls to the cast would be superfluous. ?Such calls can be
> > removed, reduced to RelabelType expressions, or annotated (via a new field 
> > in
> > FuncExpr) with the applicable exemptions. ?I modify various parse_coerce.c
> > functions to retrieve, call, and act on these exemptor functions; this 
> > includes
> > GetCoerceExemptions() from the last patch. ?I did opt to make
> > find_typmod_coercion_function return COERCION_PATH_RELABELTYPE when no work 
> > is
> > needed, rather than COERCION_PATH_NONE; this makes it consistent with
> > find_coercion_pathway's use of that enumeration.
> >
> > To demonstrate the functionality, I add exemptor functions for varchar and 
> > xml.
> > Originally I was only going to start with varchar, but xml tests the other 
> > major
> > code path, and the exemptor function for xml is dead simple.
> >
> > This helps on conversions like varchar(4)->varchar(8) and text->xml.
> 
> I've read through this patch somewhat.  As I believe Tom also
> commented previously, exemptor is a pretty bad choice of name.

I spent awhile with a thesaurus before coining that.  Since Tom's comment, I've
been hoping the next person to complain would at least suggest a better name.
Shooting in the dark on a nomenclature issue isn't fun, and I'm not picky.

> More
> generally, I think this patch is a case of coding something
> complicated without first achieving consensus on what the behavior
> should be.  I think we need to address that problem first, and then
> decide what code needs to be written, and then write the code.

Well, that's why I started the design thread "Avoiding rewrite in ALTER TABLE
ALTER TYPE" before writing any code.  That thread petered out rather than reach
any consensus.  What would you have done then?

> It seems to me that patch two in this series has the right idea: skip
> rewriting the table in cases where the types are binary coercible
> (WITHOUT FUNCTION) or one is an unconstrained domain over the other.
> IIUC, the problem this patch is trying to address is that our current
> CAST infrastructure is insufficient to identify all cases where this
> is true - in particular, it makes the decision solely based on the
> type OIDs, without consideration of the typmods.   In general, varchar
> -> varchar is not binary coercible, but varchar(4) -> varchar(8) is
> binary coercible.  I think what this patch is trying to do is tag the
> call to the cast function with the information that we might not need
> to call it, but ISTM it would be better to just notice that the
> function call is unnecessary and insert a RelabelType node instead.

This patch does exactly that for varchar(4) -> varchar(8) and similar cases.

> *reads archives*
> 
> In fact, Tom already made pretty much exactly this suggestion, on a
> thread entitled "Avoiding rewrite in ALTER TABLE ALTER TYPE".  The
> only possible objection I can see to that line of attack is that it's
> not clear what the API should be, or whether there might be too much
> runtime overhead for the amount of benefit that can be obtained.

I believe the patch does implement Tom's suggestion, at least in spirit.  He
mentioned expression_planner() as the place to do it.  In principle, We could do
it *right there* and avoid any changes to coerce_to_target_type(), but the
overhead would increase.  Specifically, we would be walking an already-built
expression tree looking for casts to remove, probably doing a syscache lookup
for every cast to grab the exemptor function OID.  Doing it there would prevent
us from directly helping or harming plain old queries.  Since ExecRelCheck(),
FormIndexDatum() and other notable paths call expression_planner(), we wouldn't
want to casually add an expensive algorithm there.  To avoid the extra syscache
lookups, we'd piggyback off those already done in coerce_to_target_type().  That
brings us to the current implementation.  Better to make the entire decision in
coerce_to_target_type() and never add the superfluous node to the expression
tree in the first place.

coerce_to_target_type() and callees seemed like the natural, low cost place to
make the changes.  By doing it that way, did I miss some important detail or
implication of Tom's suggestion?

> This is, incidentally, another problem with this patch - if you want
> to make wide-ranging changes to the system like this, you need to
> provide a convincing argument that it's not going to compromise
> correctness or performance.  Just submitting the patch without making
> an argument for why it's correct is no good, unless it's so obviously
> correct as to be unquestionable, which is certainly not the case here.
>  You've got to write up some kind of submission notes so that the
> reviewer isn't trying to guess why you think this is the right
> appro

Re: [HACKERS] Allowing multiple concurrent base backups

[Fujii Masao]

On Tue, Jan 25, 2011 at 6:02 AM, Heikki Linnakangas
 wrote:
> Hmm, perhaps the code would be more readable if instead of the
> forcePageWrites counter that counts exclusive and non-exclusive backups, and
> an exclusiveBackup boolean indicating if one of the in-progress backups is
> an exclusive one, we had a counter that only counts non-exclusive backups,
> plus a boolean indicating if an exclusive backup is in progress in addition
> to them.
>
> Attached is a patch for that (against master branch, including only xlog.c).

I read this patch and previous-posted one. Those look good.

Comments:

+ * do_pg_start_backup is the workhorse of the user-visible pg_stop_backup()
+ * function.

Typo: s/do_pg_start_backup/do_pg_stop_backup

It's helpful to explain about this behavior in pg_basebackup.sgml or elsewhere.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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] Allowing multiple concurrent base backups

[Fujii Masao]

On Tue, Jan 25, 2011 at 5:14 AM, Heikki Linnakangas
 wrote:
>> I'm not entirely sure the replication privilege removal is correct.
>> Doing that, it's no longer possible to deploy a slave *without* using
>> pg_basebackup, unless you are superuser. Do we really want to put that
>> restriction back in?
>
> Hmm, I thought we do, I thought that was changed just to make pg_basebackup
> work without superuser privileges.

If we encourage users not to use the "replication" privilege to log in
the database, putting that restriction seems to be reasonable.

> Ok, I won't touch that. But then we'll need to decide what to do about
> Fujii's observation
> (http://archives.postgresql.org/pgsql-hackers/2011-01/msg01934.php):

Yes. If we allow the "replication" users to call pg_start/stop_backup,
we also allow them to connect to the database even during shutdown
in order to cancel the backup.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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] pgindent and line length

[Tom Lane]

Bruce Momjian  writes:
> Peter Eisentraut wrote:
>> Somehow, pgindent appears to have this approach to push lines that are
>> longer than the defined line length limit back to the left to make it
>> fit.  That results in something like this:
>> 
>> Can we get rid of this behavior?

> I think we will have to switch to another indent binary to do that
> because it is done by BSD indent and I can't think of a way of fixing it
> with pre or post processing.

Whether it's fixable or not, I'm going to vote against any change that
will result in any significant number of whitespace changes, because the
pain that will create for back-patching will far outweigh any minor
visual improvements.  I'm still carrying the scars from dealing with
the ill-considered change in comment wrapping rules back around 8.1.
That caused me significant hassle on a regular basis for *years*.

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] pgindent and line length

[Bruce Momjian]

Peter Eisentraut wrote:
> Somehow, pgindent appears to have this approach to push lines that are
> longer than the defined line length limit back to the left to make it
> fit.  That results in something like this:
> 
> printfPQExpBuffer(&buf,
>   "SELECT n.nspname as \"%s\",\n"
>   "  p.proname AS \"%s\",\n"
>  "  pg_catalog.format_type(p.prorettype, NULL) AS \"%s\",\n",
>   gettext_noop("Schema"),
>   gettext_noop("Name"),
>   gettext_noop("Result data type"));
> 
> where the third line of the string was moved to the left.
> 
> I don't think this behavior is useful.  If the line is too long and
> cannot be split, then it is still more useful to be able locate the
> start of the line in an expected position than to have the end of the
> line on the hypothetical screen.  You'll have to scroll right anyway
> because there are usually lines that are going to be too long anyway.
> 
> Can we get rid of this behavior?

I think we will have to switch to another indent binary to do that
because it is done by BSD indent and I can't think of a way of fixing it
with pre or post processing.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] Seeking Mentors for Funded Reviewers

[Richard Broersma]

On Mon, Jan 24, 2011 at 5:53 PM, Josh Berkus  wrote:

> On 1/24/11 12:17 PM, Richard Broersma wrote:
> > PgUS is preparing to fund a grant for PgUS members to learn and
> > participate in the patch review process.  We looking for experienced
> > reviewers that can assist a candidate through to process of testing a
> > patch - to submitting the final review.  The ultimate deliverable
> > would be the actual review posted to Hackers.
> >
> > Would anyone be available to assist with this?
>
> Do we have candidate mentees?
>
>
Not at the moment.  Were still in the process of getting ready.

We have the funding.  We're looking for mentors.  Next we'll just about
ready to open the application process.  But I'd expect several weeks to pass
before have ready to look at applicants.

-- 
Regards,
Richard Broersma Jr.

Re: [HACKERS] Add ENCODING option to COPY

[Itagaki Takahiro]

On Sat, Jan 15, 2011 at 02:25, Hitoshi Harada  wrote:
> The patch overrides client_encoding by the added ENCODING option, and
> restores it as soon as copy is done.

We cannot do that because error messages should be encoded in the original
encoding even during COPY commands with encoding option. Error messages
could contain non-ASCII characters if lc_messages is set.

> I see some complaints ask to use
> pg_do_encoding_conversion() instead of
> pg_client_to_server/server_to_client(), but the former will surely add
> slight overhead per reading line

If we want to reduce the overhead, we should cache the conversion procedure
in CopyState. How about adding something like "FmgrInfo file_to_server_covv"
into it?

-- 
Itagaki Takahiro

-- 
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] Seeking Mentors for Funded Reviewers

[Josh Berkus]

On 1/24/11 12:17 PM, Richard Broersma wrote:
> PgUS is preparing to fund a grant for PgUS members to learn and
> participate in the patch review process.  We looking for experienced
> reviewers that can assist a candidate through to process of testing a
> patch - to submitting the final review.  The ultimate deliverable
> would be the actual review posted to Hackers.
> 
> Would anyone be available to assist with this?

Do we have candidate mentees?

-- 
  -- Josh Berkus
 PostgreSQL Experts Inc.
 http://www.pgexperts.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Change pg_last_xlog_receive_location not to move backwards

[Fujii Masao]

On Tue, Jan 25, 2011 at 4:25 AM, Robert Haas  wrote:
> On Thu, Jan 13, 2011 at 2:40 AM, Fujii Masao  wrote:
>> On Thu, Jan 13, 2011 at 4:24 PM, Fujii Masao  wrote:
>>> So I'm thinking to change pg_last_xlog_receive_location not to
>>> move backwards.
>>
>> The attached patch does that.
>
> It looks to me like this is changing more than just the return value
> of pg_last_xlog_receive_location.  receivedUpto isn't only used for
> that one function, and there's no explanation in your email or in the
> patch of why the new behavior is correct and/or better for the other
> places where it's used.
>
> This email from Heikki seems to indicate that there's a reason for the
> current behavior:
>
> http://archives.postgresql.org/pgsql-hackers/2010-06/msg00586.php

Yes, so I didn't change that behavior, i.e., even with the patch, SR still
always starts from the head of the WAL segment, not the middle of that.
What I changed is only the return value of pg_last_xlog_receive_location.
Am I missing something?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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] Per-column collation, the finale

[Itagaki Takahiro]

On Sat, Jan 15, 2011 at 06:41, Peter Eisentraut  wrote:
> I've been going over this patch with a fine-tooth comb for the last two
> weeks, fixed some small bugs, added comments, made initdb a little
> friendlier, but no substantial changes.

What can I do to test the patch?
No regression tests are included in it, and I have an almost empty
pg_collation catalog with it:

=# SELECT * FROM pg_collation;
 collname | collnamespace | collencoding | collcollate | collctype
--+---+--+-+---
 default  |11 |0 | |
(1 row)

-- 
Itagaki Takahiro

-- 
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] Re: patch: fix performance problems with repated decomprimation of varlena values in plpgsql

[Noah Misch]

On Sat, Jan 22, 2011 at 11:32:02AM +0100, Pavel Stehule wrote:
> because I am not sure so any complex solution can be done to deadline
> for 9.1, I created a patch that is based on Tom ideas - just
> explicitly detoast function parameters.

I can confirm that, for your original test case, this yields performance
comparable to that of your original patch.

This patch hooks into plpgsql_exec_function, detoasting only the function
arguments.  Therefore, it doesn't help in a test case like the one I posted in
my original review.  That test case initialized a variable using SELECT INTO,
then used the variable in a loop.  Is there any benefit to doing this in
plpgsql_exec_function, versus exec_assign_value (Tom's suggestion), which would
presumably help the other test case also?

As we've discussed, unlike the original patch, this yields similarly grand
performance regressions on functions that receive toasted arguments and never
use them.  Who is prepared to speculate that this will help more people than it
will hurt?  This patch is easier on -hackers than the original, but it seems
much more likely to create measurable performance regressions in the field.
It's clear the committers prefer it this way, but I remain skeptical.

nm

-- 
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 to add a primary key using an existing index

[Robert Haas]

On Mon, Jan 24, 2011 at 7:01 PM, Tom Lane  wrote:
> One other issue that might be worthy of discussion is that as things
> stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause
> the constraint to absorb the index as an INTERNAL dependency.  That
> means dropping the constraint would make the index go away silently ---
> it no longer has any separate life. If the intent is just to provide a
> way to get the effect of ALTER ADD PRIMARY KEY CONCURRENTLY, then this
> behavior is probably fine.  But someone who believes DROP CONSTRAINT
> exactly reverses the effects of ADD CONSTRAINT might be surprised.
> Comments?

Well, I think the behavior as described is what we want.  If the
syntax associated with that behavior is going to lead to confusion,
I'd view that as a deficiency of the syntax, rather than a deficiency
of the behavior.  (I make this comment with some reluctance
considering the amount of bikeshedding we've already done on this
topic, but... that's what I think.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] pg_test_fsync problem

[Bruce Momjian]

Tom Lane wrote:
> I wrote:
> > He's complaining that it dies with EINVAL.
> 
> > I notice that (1) it's using O_DIRECT even though the printout claims
> > otherwise, and (2) it's writing from a buffer that has no better than
> > char alignment, which is certainly not OK for O_DIRECT.  Either one
> > of those could plausibly result in EINVAL ...
> 
> Oh, scratch that: the buffer is properly aligned, it's the length that's
> bogus for O_DIRECT.  I rather imagine that test_open_sync is meant to be
> writing so many kilobytes, not so many bytes.

Yes, that was a bug in my code that I have fixed with the attached,
applied patch.  Thanks for the report.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_test_fsync/pg_test_fsync.c b/contrib/pg_test_fsync/pg_test_fsync.c
index 7ece9b9..d8099a5 100644
*** /tmp/pgdiff.10514/8NsZJd_pg_test_fsync.c	Mon Jan 24 19:41:20 2011
--- contrib/pg_test_fsync/pg_test_fsync.c	Mon Jan 24 19:39:48 2011
*** test_open_sync(const char *msg, int writ
*** 421,427 
  		for (ops = 0; ops < ops_per_test; ops++)
  		{
  			for (writes = 0; writes < 16 / writes_size; writes++)
! if (write(tmpfile, buf, writes_size) != writes_size)
  	die("write failed");
  			if (lseek(tmpfile, 0, SEEK_SET) == -1)
  die("seek failed");
--- 421,428 
  		for (ops = 0; ops < ops_per_test; ops++)
  		{
  			for (writes = 0; writes < 16 / writes_size; writes++)
! if (write(tmpfile, buf, writes_size * 1024) !=
! 	writes_size * 1024)
  	die("write failed");
  			if (lseek(tmpfile, 0, SEEK_SET) == -1)
  die("seek failed");

-- 
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] Use of O_DIRECT only for open_* sync options

[Bruce Momjian]

Greg Smith wrote:
> Bruce Momjian wrote:
> > xlogdefs.h says:
> >
> > /*
> >  *  Because O_DIRECT bypasses the kernel buffers, and because we never
> >  *  read those buffers except during crash recovery, it is a win to use
> >  *  it in all cases where we sync on each write().  We could allow O_DIRECT
> >  *  with fsync(), but because skipping the kernel buffer forces writes out
> >  *  quickly, it seems best just to use it for O_SYNC.  It is hard to imagine
> >  *  how fsync() could be a win for O_DIRECT compared to O_SYNC and O_DIRECT.
> >  *  Also, O_DIRECT is never enough to force data to the drives, it merely
> >  *  tries to bypass the kernel cache, so we still need O_SYNC or fsync().
> >  */
> >
> > This seems wrong because fsync() can win if there are two writes before
> > the sync call.  Can kernels not issue fsync() if the write was O_DIRECT?
> > If that is the cause, we should document it.
> >   
> 
> The comment does look busted, because you did imagine exactly a case 
> where they might be combined.  The only incompatibility that I'm aware 
> of is that O_DIRECT requires reads and writes to be aligned properly, so 
> you can't use it in random application code unless it's aware of that.  
> O_DIRECT and fsync are compatible; for example, MySQL allows combining 
> the two:  http://dev.mysql.com/doc/refman/5.1/en/innodb-parameters.html
> 
> (That whole bit of documentation around innodb_flush_method includes 
> some very interesting observations around O_DIRECT actually)
> 
> I'm starting to consider the idea that much of the performance gains 
> seen on earlier systems with O_DIRECT was because it decreased CPU usage 
> shuffling things into the OS cache, rather than its impact on avoiding 
> pollution of said cache.  On Linux for example, its main accomplishment 
> is decribed like this:  "File I/O is done directly to/from user space 
> buffers."  
> http://www.kernel.org/doc/man-pages/online/pages/man2/open.2.html  The 
> earliest paper on the implementation suggests a big decrease in CPU 
> overhead from that:  
> http://www.ukuug.org/events/linux2001/papers/html/AArcangeli-o_direct.html
> 
> Impossible to guess whether that's more true ("CPU cache pollution is a 
> bigger problem now") or less true ("drives are much slower relative to 
> CPUs now") today.  I'm trying to remain agnostic and let the benchmarks 
> offer an opinion instead.

Agreed.  Perhaps we need a separate setting to turn direct I/O on and
off, and decouple wal_sync_method and direct I/O.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
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] ALTER TYPE 3: add facility to identify further no-work cases

[Robert Haas]

On Sun, Jan 9, 2011 at 5:03 PM, Noah Misch  wrote:
> Here I add the notion of an "exemptor function", a property of a cast that
> determines when calls to the cast would be superfluous.  Such calls can be
> removed, reduced to RelabelType expressions, or annotated (via a new field in
> FuncExpr) with the applicable exemptions.  I modify various parse_coerce.c
> functions to retrieve, call, and act on these exemptor functions; this 
> includes
> GetCoerceExemptions() from the last patch.  I did opt to make
> find_typmod_coercion_function return COERCION_PATH_RELABELTYPE when no work is
> needed, rather than COERCION_PATH_NONE; this makes it consistent with
> find_coercion_pathway's use of that enumeration.
>
> To demonstrate the functionality, I add exemptor functions for varchar and 
> xml.
> Originally I was only going to start with varchar, but xml tests the other 
> major
> code path, and the exemptor function for xml is dead simple.
>
> This helps on conversions like varchar(4)->varchar(8) and text->xml.

I've read through this patch somewhat.  As I believe Tom also
commented previously, exemptor is a pretty bad choice of name.  More
generally, I think this patch is a case of coding something
complicated without first achieving consensus on what the behavior
should be.  I think we need to address that problem first, and then
decide what code needs to be written, and then write the code.

It seems to me that patch two in this series has the right idea: skip
rewriting the table in cases where the types are binary coercible
(WITHOUT FUNCTION) or one is an unconstrained domain over the other.
IIUC, the problem this patch is trying to address is that our current
CAST infrastructure is insufficient to identify all cases where this
is true - in particular, it makes the decision solely based on the
type OIDs, without consideration of the typmods.   In general, varchar
-> varchar is not binary coercible, but varchar(4) -> varchar(8) is
binary coercible.  I think what this patch is trying to do is tag the
call to the cast function with the information that we might not need
to call it, but ISTM it would be better to just notice that the
function call is unnecessary and insert a RelabelType node instead.

*reads archives*

In fact, Tom already made pretty much exactly this suggestion, on a
thread entitled "Avoiding rewrite in ALTER TABLE ALTER TYPE".  The
only possible objection I can see to that line of attack is that it's
not clear what the API should be, or whether there might be too much
runtime overhead for the amount of benefit that can be obtained.

This is, incidentally, another problem with this patch - if you want
to make wide-ranging changes to the system like this, you need to
provide a convincing argument that it's not going to compromise
correctness or performance.  Just submitting the patch without making
an argument for why it's correct is no good, unless it's so obviously
correct as to be unquestionable, which is certainly not the case here.
 You've got to write up some kind of submission notes so that the
reviewer isn't trying to guess why you think this is the right
approach, or spending a lot of time thinking through approaches you've
discarded for good reason.  If there is a danger of performance
regression, you need to refute it, preferably with actual benchmarks.
A particular aspect I'm concerned about with this patch in particular:
it strikes me that the overhead of calling the exemptor functions in
the ALTER TABLE case is negligible, but that this might not be true in
other contexts where some of this logic may get invoked - it appears
to implicate the casting machinery much more generally.

I think it's a mistake to merge the handling of the rewrite-vs-noop
case with the check-vs-noop case.  They're fundamentally dissimilar.
As noted above, being able to notice the noop case seems like it could
save run-time work during ordinary query execution.  But detecting the
"check" case is useless work except in the specific case of a table
rewrite.  If we're going to do that at all, it seems to me that it
needs to be done via a code-path that's only going to get invoked in
the case of ALTER TABLE, not something that's going to happen every
time we parse an expression tree.  But it seems to me that it might be
better to take the suggestion I made before: forget about the
check-only case for now, and focus on the do-nothing case.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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 to add a primary key using an existing index

[Tom Lane]

I wrote:
> ... If that's the only issue then I don't see any need to wait on
> the author, so will take this one.

I find myself quite dissatisfied with the way that this patch adds yet
another bool flag to index_create (which has too many of those already),
with the effect of causing it to exactly *not* do an index creation.
That's a clear violation of the principle of least astonishment IMNSHO.
I think what's needed here is to refactor things a bit so that the
constraint-creation code is pulled out of index_create and called
separately where needed.  Hacking on that now.

One other issue that might be worthy of discussion is that as things
stand, execution of the ADD CONSTRAINT USING INDEX syntax will cause
the constraint to absorb the index as an INTERNAL dependency.  That
means dropping the constraint would make the index go away silently ---
it no longer has any separate life.  If the intent is just to provide a
way to get the effect of ALTER ADD PRIMARY KEY CONCURRENTLY, then this
behavior is probably fine.  But someone who believes DROP CONSTRAINT
exactly reverses the effects of ADD CONSTRAINT might be surprised.
Comments?

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

[HACKERS] pgindent and line length

[Peter Eisentraut]

Somehow, pgindent appears to have this approach to push lines that are
longer than the defined line length limit back to the left to make it
fit.  That results in something like this:

printfPQExpBuffer(&buf,
  "SELECT n.nspname as \"%s\",\n"
  "  p.proname AS \"%s\",\n"
 "  pg_catalog.format_type(p.prorettype, NULL) AS \"%s\",\n",
  gettext_noop("Schema"),
  gettext_noop("Name"),
  gettext_noop("Result data type"));

where the third line of the string was moved to the left.

I don't think this behavior is useful.  If the line is too long and
cannot be split, then it is still more useful to be able locate the
start of the line in an expected position than to have the end of the
line on the hypothetical screen.  You'll have to scroll right anyway
because there are usually lines that are going to be too long anyway.

Can we get rid of this behavior?



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] SSI future enhancement: de facto read only optimization

[Kevin Grittner]

Just to put this on the record for 9.2: I've just thought of a way
to further extend the READ ONLY optimizations to de facto READ ONLY
transactions.  Right now, when a dangerous structure is formed with
three transactions (which consists of read-write dependencies from
T0 -> T1 -> T2) and T2 is committed, we have an optimization where
we recognize that nothing needs to be rolled back if T0 is READ ONLY
and overlaps T2.  We could extend this to de facto READ ONLY
transactions (those which don't write to a permanent table, but fail
to declare that up-front) where T0 overlaps T2 by holding off on
rolling anything back until T2 actually does a write.  We already
have flags which track the relevant data, and we already use them to
optimize de facto read only transactions where T0 commits before T1;
but we could avoid a few false positives with this additional
optimization.
 
Definitely too late in the release cycle to be trying to add a new
optimization, though.
 
-Kevin

-- 
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] Allowing multiple concurrent base backups

[Heikki Linnakangas]

On 24.01.2011 22:31, Magnus Hagander wrote:

On Mon, Jan 24, 2011 at 21:14, Heikki Linnakangas
  wrote:

On 24.01.2011 20:22, Magnus Hagander wrote:

I can't see an explicit check for the user ttempting to do
pg_stop_backup() when there is a nonexclusive backup running? Maybe
I'm reading it wrong? The case being when a user has started a backup
with pg_basebackup but then connects and manually does a
pg_stop_backup. ISTM it drops us ina codepath that just doesn't do the
decrement, but also doesn't throw an error?


It throws an error later when it won't find backup_label. For better or
worse, it's always been like that.


Isn't that going to leave us in a broken state though? As in a
mistaken pg_stop_backup() will decrement the counter both breaking the
streaming base backup, and also possibly throwing an assert when that
one eventually wants to do it's do_pg_stop_backup()?


Umm, no. pg_stop_backup() won't decrement the counter unless there's an 
exclusive backup in operation according to the exclusiveBackup flag.


Hmm, perhaps the code would be more readable if instead of the 
forcePageWrites counter that counts exclusive and non-exclusive backups, 
and an exclusiveBackup boolean indicating if one of the in-progress 
backups is an exclusive one, we had a counter that only counts 
non-exclusive backups, plus a boolean indicating if an exclusive backup 
is in progress in addition to them.


Attached is a patch for that (against master branch, including only xlog.c).

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***
*** 60,67 
  
  
  /* File path names (all relative to $PGDATA) */
- #define BACKUP_LABEL_FILE		"backup_label"
- #define BACKUP_LABEL_OLD		"backup_label.old"
  #define RECOVERY_COMMAND_FILE	"recovery.conf"
  #define RECOVERY_COMMAND_DONE	"recovery.done"
  
--- 60,65 
***
*** 339,344  typedef struct XLogCtlInsert
--- 337,351 
  	char	   *currpos;		/* current insertion point in cache */
  	XLogRecPtr	RedoRecPtr;		/* current redo point for insertions */
  	bool		forcePageWrites;	/* forcing full-page writes for PITR? */
+ 
+ 	/*
+ 	 * exclusiveBackup is true if a backup started with pg_start_backup() is
+ 	 * in progress, and nonExclusiveBackups is a counter indicating the number
+ 	 * of streaming base backups currently in progress. forcePageWrites is
+ 	 * set to true, when either of these is non-zero.
+ 	 */
+ 	bool		exclusiveBackup;
+ 	int			nonExclusiveBackups;
  } XLogCtlInsert;
  
  /*
***
*** 8352,8367  pg_start_backup(PG_FUNCTION_ARGS)
  
  	backupidstr = text_to_cstring(backupid);
  
! 	startpoint = do_pg_start_backup(backupidstr, fast);
  
  	snprintf(startxlogstr, sizeof(startxlogstr), "%X/%X",
  			 startpoint.xlogid, startpoint.xrecoff);
  	PG_RETURN_TEXT_P(cstring_to_text(startxlogstr));
  }
  
  XLogRecPtr
! do_pg_start_backup(const char *backupidstr, bool fast)
  {
  	XLogRecPtr	checkpointloc;
  	XLogRecPtr	startpoint;
  	pg_time_t	stamp_time;
--- 8359,8396 
  
  	backupidstr = text_to_cstring(backupid);
  
! 	startpoint = do_pg_start_backup(backupidstr, fast, NULL);
  
  	snprintf(startxlogstr, sizeof(startxlogstr), "%X/%X",
  			 startpoint.xlogid, startpoint.xrecoff);
  	PG_RETURN_TEXT_P(cstring_to_text(startxlogstr));
  }
  
+ /*
+  * do_pg_start_backup is the workhorse of the user-visible pg_start_backup()
+  * function. It creates the necessary starting checkpoint and constructs the
+  * backup label file.
+  * 
+  * There are two kind of backups: exclusive and non-exclusive. An exclusive
+  * backup is started with pg_start_backup(), and there can be only one active
+  * at a time. The backup label file of an exclusive backup is written to
+  * $PGDATA/backup_label, and it is removed by pg_stop_backup().
+  *
+  * A non-exclusive backup is used for the streaming base backups (see
+  * src/backend/replication/basebackup.c). The difference to exclusive backups
+  * is that the backup label file is not written to disk. Instead, its would-be
+  * contents are returned in *labelfile, and the caller is responsible for
+  * including it in the backup archive as 'backup_label'. There can be many
+  * non-exclusive backups active at the same time, and they don't conflict
+  * with an exclusive backup either.
+  *
+  * Every successfully started non-exclusive backup must be stopped by calling
+  * do_pg_stop_backup() or do_pg_abort_backup().
+  */
  XLogRecPtr
! do_pg_start_backup(const char *backupidstr, bool fast, char **labelfile)
  {
+ 	bool		exclusive = (labelfile == NULL);
  	XLogRecPtr	checkpointloc;
  	XLogRecPtr	startpoint;
  	pg_time_t	stamp_time;
***
*** 8371,8381  do_pg_start_backup(const char *backupidstr, bool fast)
  	uint32		_logSeg;
  	struct stat stat_buf;
  	FILE	   *fp;
  
! 	if (!superuser() && !is_authenticated_user_replication_role())
  		ereport(ERROR,
  (errcode(ERRCODE_INSUFFIC

Re: [HACKERS] Allowing multiple concurrent base backups

[Magnus Hagander]

On Mon, Jan 24, 2011 at 21:14, Heikki Linnakangas
 wrote:
> On 24.01.2011 20:22, Magnus Hagander wrote:
>> I can't see an explicit check for the user ttempting to do
>> pg_stop_backup() when there is a nonexclusive backup running? Maybe
>> I'm reading it wrong? The case being when a user has started a backup
>> with pg_basebackup but then connects and manually does a
>> pg_stop_backup. ISTM it drops us ina codepath that just doesn't do the
>> decrement, but also doesn't throw an error?
>
> It throws an error later when it won't find backup_label. For better or
> worse, it's always been like that.

Isn't that going to leave us in a broken state though? As in a
mistaken pg_stop_backup() will decrement the counter both breaking the
streaming base backup, and also possibly throwing an assert when that
one eventually wants to do it's do_pg_stop_backup()?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Seeking Mentors for Funded Reviewers

[Richard Broersma]

PgUS is preparing to fund a grant for PgUS members to learn and
participate in the patch review process.  We looking for experienced
reviewers that can assist a candidate through to process of testing a
patch - to submitting the final review.  The ultimate deliverable
would be the actual review posted to Hackers.

Would anyone be available to assist with this?

Thoughts?

-- 
Regards,
Richard Broersma Jr.

-- 
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] Allowing multiple concurrent base backups

[Heikki Linnakangas]

On 24.01.2011 20:22, Magnus Hagander wrote:

On Mon, Jan 24, 2011 at 17:52, Heikki Linnakangas
  wrote:

Another updated patch. Fixed bitrot, and addressed the privilege issue
Fujii-san raised here:
http://archives.postgresql.org/message-id/4d380560.3040...@enterprisedb.com.
I changed the privilege checks so that pg_start/stop_backup() functions
require superuser privileges again, but not for a base backup via the
replication protocol (replication privilege is needed to establish a
replication connection to begin with).


I'm not entirely sure the replication privilege removal is correct.
Doing that, it's no longer possible to deploy a slave *without* using
pg_basebackup, unless you are superuser. Do we really want to put that
restriction back in?


Hmm, I thought we do, I thought that was changed just to make 
pg_basebackup work without superuser privileges. But I guess it makes 
sense apart from that. So the question is, should 'replication' 
privilege be enough to use pg_start/stop_backup(), or should we require 
superuser access for that? In any case, replication privilege will be 
enough for pg_basebackup.


Ok, I won't touch that. But then we'll need to decide what to do about 
Fujii's observation 
(http://archives.postgresql.org/pgsql-hackers/2011-01/msg01934.php):

Both the user with REPLICATION privilege and the superuser can
call pg_stop_backup. But only superuser can connect to the server
to cancel online backup during shutdown. The non-superuser with
REPLICATION privilege cannot. Is this behavior intentional? Or just
oversight?




I can't see an explicit check for the user ttempting to do
pg_stop_backup() when there is a nonexclusive backup running? Maybe
I'm reading it wrong? The case being when a user has started a backup
with pg_basebackup but then connects and manually does a
pg_stop_backup. ISTM it drops us ina codepath that just doesn't do the
decrement, but also doesn't throw an error?


It throws an error later when it won't find backup_label. For better or 
worse, it's always been like that.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] READ ONLY fixes

[Robert Haas]

On Mon, Jan 24, 2011 at 2:21 PM, Kevin Grittner
 wrote:
> Robert Haas  wrote:
>
>> I am wondering if it wouldn't be simpler and more logical to allow
>> idempotent changes of these settings at any time, and to restrict
>> only changes that actually change something.  It feels really
>> weird to allow changing these properties to their own values at
>> any time within a subtransaction, but not in a top-level
>> transaction.
>
> I just looked at the committed code, and saw that it not only
> changed things in this regard, but also allows a change from READ
> WRITE to READ ONLY at any point in a transaction.  (I see now that
> your pseudo-code did the same, but I didn't pick up on it at the
> time.)
>
> That part of it seems a little weird to me.  I think I can live with
> it since it doesn't open up any routes to break SSI (now that I
> reviewed our use of XactReadOnly and tweaked a function), or to
> subvert security except for the unlikely scenario that something is
> checking RO state and depending on there having been no writes
> earlier in the transaction -- in which case they'd still need to be
> very careful about subtransactions.
>
> In short, I'm OK with it but wanted to make sure the community was
> aware of the change to what this patch was doing, because I don't
> think the discussion made that entirely clear.

Hmm, sorry, I thought that had been made clear.  I guess the issue is
that within a subtransaction we can't really prohibit that anyway, so
spending extra code to do it in a toplevel transaction seems like
making the code more complicated just for the heck of it.  I wasn't
intending to do anything not agreed...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Change pg_last_xlog_receive_location not to move backwards

[Robert Haas]

On Thu, Jan 13, 2011 at 2:40 AM, Fujii Masao  wrote:
> On Thu, Jan 13, 2011 at 4:24 PM, Fujii Masao  wrote:
>> So I'm thinking to change pg_last_xlog_receive_location not to
>> move backwards.
>
> The attached patch does that.

It looks to me like this is changing more than just the return value
of pg_last_xlog_receive_location.  receivedUpto isn't only used for
that one function, and there's no explanation in your email or in the
patch of why the new behavior is correct and/or better for the other
places where it's used.

This email from Heikki seems to indicate that there's a reason for the
current behavior:

http://archives.postgresql.org/pgsql-hackers/2010-06/msg00586.php

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] READ ONLY fixes

[Kevin Grittner]

Robert Haas  wrote:
 
> I am wondering if it wouldn't be simpler and more logical to allow
> idempotent changes of these settings at any time, and to restrict
> only changes that actually change something.  It feels really
> weird to allow changing these properties to their own values at
> any time within a subtransaction, but not in a top-level
> transaction.
 
I just looked at the committed code, and saw that it not only
changed things in this regard, but also allows a change from READ
WRITE to READ ONLY at any point in a transaction.  (I see now that
your pseudo-code did the same, but I didn't pick up on it at the
time.)
 
That part of it seems a little weird to me.  I think I can live with
it since it doesn't open up any routes to break SSI (now that I
reviewed our use of XactReadOnly and tweaked a function), or to
subvert security except for the unlikely scenario that something is
checking RO state and depending on there having been no writes
earlier in the transaction -- in which case they'd still need to be
very careful about subtransactions.
 
In short, I'm OK with it but wanted to make sure the community was
aware of the change to what this patch was doing, because I don't
think the discussion made that entirely clear.
 
-Kevin

-- 
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] Allowing multiple concurrent base backups

[Magnus Hagander]

On Mon, Jan 24, 2011 at 17:52, Heikki Linnakangas
 wrote:
> On 13.01.2011 23:32, Heikki Linnakangas wrote:
>>
>> Anyway, here's an updated patch with all the known issues fixed.
>
> Another updated patch. Fixed bitrot, and addressed the privilege issue
> Fujii-san raised here:
> http://archives.postgresql.org/message-id/4d380560.3040...@enterprisedb.com.
> I changed the privilege checks so that pg_start/stop_backup() functions
> require superuser privileges again, but not for a base backup via the
> replication protocol (replication privilege is needed to establish a
> replication connection to begin with).

I'm not entirely sure the replication privilege removal is correct.
Doing that, it's no longer possible to deploy a slave *without* using
pg_basebackup, unless you are superuser. Do we really want to put that
restriction back in?

(And if we do, the docs proably need an update...)

I can't see an explicit check for the user ttempting to do
pg_stop_backup() when there is a nonexclusive backup running? Maybe
I'm reading it wrong? The case being when a user has started a backup
with pg_basebackup but then connects and manually does a
pg_stop_backup. ISTM it drops us ina codepath that just doesn't do the
decrement, but also doesn't throw an error?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch to add a primary key using an existing index

[Tom Lane]

Steve Singer  writes:
> src/backend/parser/parse_utilcmd.c: 1452
> Your calling strdup on the attribute name.  I don't have a good enough 
> grasp on the code to be able to trace this through to where the memory 
> gets free'd.  Does it get freed? Should/could this be a call to pstrdup

strdup() is pretty much automatically wrong in the parser, not to
mention most of the rest of the backend.  pstrdup is likely what was
meant.  If that's the only issue then I don't see any need to wait on
the author, so will take this one.

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] wildcard search support for pg_trgm

[Tom Lane]

Jesper Krogh  writes:
> Would it be hard to make it support "n-grams" (e.g. making the length
> configurable) instead of trigrams?

That would be a complete rewrite with an incompatible on-disk index
representation, which seems a bit beyond the scope of this patch.

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] ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED

[Chris Browne]

si...@2ndquadrant.com (Simon Riggs) writes:
> I just wanted to point out that the patch submitted here does not allow
> what is requested here for FKs (nor indexes).

That's fine; I was trying to support the thought that there was
something useful about this idea.  Being able to expressly deactivate
indices seems like a reasonable thing to add as a separate TODO item,
and I'll see about doing so.
-- 
MICROS~1 has  brought the  microcomputer OS to  the point where  it is
more bloated than even OSes from what was previously larger classes of
machines   altogether.   This  is   perhaps  Bill's   single  greatest
accomplishment.

-- 
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] REVIEW: EXPLAIN and nfiltered

[Tom Lane]

Florian Pflug  writes:
> On Jan22, 2011, at 17:55 , Tom Lane wrote:
>> Reflecting on that, I'm inclined to suggest
>>  Bitmap Heap Scan ...
>>  Recheck Cond: blah blah
>>  Rows Removed by Recheck Cond: 42
>>  Filter Cond: blah blah blah
>>  Rows Removed by Filter Cond: 77

> +1. Repeating the label of the condition adds enough context to make
> "Removed" unambiguous IMHO.

Given where we've ended up on what we want printed, I'm forced to
conclude that there is basically nothing usable in the submitted patch.
We have the following problems:

1. It only instruments the ExecQual() call in execScan.c.  There are
close to 20 other calls that also need instrumentation, unless we
intentionally decide not to bother with counting results for certain
filters (but see #4).

2. Putting the counter in struct Instrumentation doesn't scale very
nicely to cases with more than one qual per node.  We could put some
arbitrary number of counters into that struct and make some arbitrary
assignments of which is used for what, but ugh.  I am tempted to suggest
that these counters should go right into the PlanState nodes for the
relevant plan node types; which would likely mean that they'd get
incremented unconditionally whether we're running EXPLAIN ANALYZE or
not.  Offhand I don't have a problem with that --- it's not apparent
to me that

if (node->ps.instrument)
node->counter += 1;

is faster than an unconditional

node->counter += 1;

on modern machines in the first place, and in the second place we have
certainly expended many more cycles than that by the time we have
completed a failing ExecQual call, so it's unlikely to matter.

3. The additions to explain.c of course need complete revision to
support this output style.  Likewise the documentation (and as was
mentioned upthread this isn't enough documentation anyway, since it
utterly fails to explain what nfiltered is to users).

4. I'm not entirely sure what to do with nodeResult's resconstantqual.
If we do instrument that, it would have to be done differently from
everywhere else, since execQual is called only once not once per tuple.
But on the whole I think we could leave it out, since it's pretty
obvious from the node's overall behavior whether the qual passed or not.


I had thought perhaps I could fix this patch up and commit it, but a
complete rewrite seems beyond the bounds of what should happen during a
CommitFest, especially since there are lots of other patches in need of
attention.  So I'm marking it Returned With Feedback.

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] Allowing multiple concurrent base backups

[Heikki Linnakangas]

On 13.01.2011 23:32, Heikki Linnakangas wrote:

Anyway, here's an updated patch with all the known issues fixed.


Another updated patch. Fixed bitrot, and addressed the privilege issue 
Fujii-san raised here: 
http://archives.postgresql.org/message-id/4d380560.3040...@enterprisedb.com. 
I changed the privilege checks so that pg_start/stop_backup() functions 
require superuser privileges again, but not for a base backup via the 
replication protocol (replication privilege is needed to establish a 
replication connection to begin with).


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***
*** 60,67 
  
  
  /* File path names (all relative to $PGDATA) */
- #define BACKUP_LABEL_FILE		"backup_label"
- #define BACKUP_LABEL_OLD		"backup_label.old"
  #define RECOVERY_COMMAND_FILE	"recovery.conf"
  #define RECOVERY_COMMAND_DONE	"recovery.done"
  
--- 60,65 
***
*** 338,344  typedef struct XLogCtlInsert
  	XLogPageHeader currpage;	/* points to header of block in cache */
  	char	   *currpos;		/* current insertion point in cache */
  	XLogRecPtr	RedoRecPtr;		/* current redo point for insertions */
! 	bool		forcePageWrites;	/* forcing full-page writes for PITR? */
  } XLogCtlInsert;
  
  /*
--- 336,343 
  	XLogPageHeader currpage;	/* points to header of block in cache */
  	char	   *currpos;		/* current insertion point in cache */
  	XLogRecPtr	RedoRecPtr;		/* current redo point for insertions */
! 	int			forcePageWrites;	/* forcing full-page writes for PITR? */
! 	bool		exclusiveBackup;	/* a backup was started with pg_start_backup() */
  } XLogCtlInsert;
  
  /*
***
*** 8352,8367  pg_start_backup(PG_FUNCTION_ARGS)
  
  	backupidstr = text_to_cstring(backupid);
  
! 	startpoint = do_pg_start_backup(backupidstr, fast);
  
  	snprintf(startxlogstr, sizeof(startxlogstr), "%X/%X",
  			 startpoint.xlogid, startpoint.xrecoff);
  	PG_RETURN_TEXT_P(cstring_to_text(startxlogstr));
  }
  
  XLogRecPtr
! do_pg_start_backup(const char *backupidstr, bool fast)
  {
  	XLogRecPtr	checkpointloc;
  	XLogRecPtr	startpoint;
  	pg_time_t	stamp_time;
--- 8351,8388 
  
  	backupidstr = text_to_cstring(backupid);
  
! 	startpoint = do_pg_start_backup(backupidstr, fast, NULL);
  
  	snprintf(startxlogstr, sizeof(startxlogstr), "%X/%X",
  			 startpoint.xlogid, startpoint.xrecoff);
  	PG_RETURN_TEXT_P(cstring_to_text(startxlogstr));
  }
  
+ /*
+  * do_pg_start_backup is the workhorse of the user-visible pg_start_backup()
+  * function. It creates the necessary starting checkpoint and constructs the
+  * backup label file.
+  * 
+  * There are two kind of backups: exclusive and non-exclusive. An exclusive
+  * backup is started with pg_start_backup(), and there can be only one active
+  * at a time. The backup label file of an exclusive backup is written to
+  * $PGDATA/backup_label, and it is removed by pg_stop_backup().
+  *
+  * A non-exclusive backup is used for the streaming base backups (see
+  * src/backend/replication/basebackup.c). The difference to exclusive backups
+  * is that the backup label file is not written to disk. Instead, its would-be
+  * contents are returned in *labelfile, and the caller is responsible for
+  * including it in the backup archive as 'backup_label'. There can be many
+  * non-exclusive backups active at the same time, and they don't conflict
+  * with an exclusive backup either.
+  *
+  * Every successfully started non-exclusive backup must be stopped by calling
+  * do_pg_stop_backup() or do_pg_abort_backup().
+  */
  XLogRecPtr
! do_pg_start_backup(const char *backupidstr, bool fast, char **labelfile)
  {
+ 	bool		exclusive = (labelfile == NULL);
  	XLogRecPtr	checkpointloc;
  	XLogRecPtr	startpoint;
  	pg_time_t	stamp_time;
***
*** 8371,8381  do_pg_start_backup(const char *backupidstr, bool fast)
  	uint32		_logSeg;
  	struct stat stat_buf;
  	FILE	   *fp;
  
! 	if (!superuser() && !is_authenticated_user_replication_role())
  		ereport(ERROR,
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
!  errmsg("must be superuser or replication role to run a backup")));
  
  	if (RecoveryInProgress())
  		ereport(ERROR,
--- 8392,8403 
  	uint32		_logSeg;
  	struct stat stat_buf;
  	FILE	   *fp;
+ 	StringInfoData labelfbuf;
  
! 	if (exclusive && !superuser())
  		ereport(ERROR,
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
!  errmsg("must be superuser to run a backup")));
  
  	if (RecoveryInProgress())
  		ereport(ERROR,
***
*** 8389,8394  do_pg_start_backup(const char *backupidstr, bool fast)
--- 8411,8422 
  			  errmsg("WAL level not sufficient for making an online backup"),
   errhint("wal_level must be set to \"archive\" or \"hot_standby\" at server start.")));
  
+ 	if (strlen(backupidstr) > MAXPGPATH)
+ 		ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg("backup

Re: [HACKERS] wildcard search support for pg_trgm

[Jesper Krogh]

On 2011-01-24 16:34, Alexander Korotkov wrote:

Hi!

On Mon, Jan 24, 2011 at 3:07 AM, Jan Urbański  wrote:


I see two issues with this patch. First of them is the resulting index
size. I created a table with 5 copies of
/usr/share/dict/american-english in it and a gin index on it, using
gin_trgm_ops. The results were:

  * relation size: 18MB
  * index size: 109 MB

while without the patch the GIN index was 43 MB. I'm not really sure
*why* this happens, as it's not obvious from reading the patch what
exactly is this extra data that gets stored in the index, making it more
than double its size.


Do you sure that you did comparison correctly? The sequence of index
building and data insertion does matter. I tried to build gin index on  5
copies of /usr/share/dict/american-english with patch and got 43 MB index
size.



That leads me to the second issue. The pg_trgm code is already woefully
uncommented, and after spending quite some time reading it back and
forth I have to admit that I don't really understand what the code does
in the first place, and so I don't understand what does that patch
change. I read all the changes in detail and I could't find any obvious
mistakes like reading over array boundaries or dereferencing
uninitialized pointers, but I can't tell if the patch is correct
semantically. All test cases I threw at it work, though.


I'll try to write sufficient comment and send new revision of patch.


Would it be hard to make it support "n-grams" (e.g. making the length
configurable) instead of trigrams? I actually had the feeling that
penta-grams (pen-tuples or whatever they would be called) would
be better for my usecase (large substring-search in large documents ..
eg. 500 within 3.000.

Larger sizes.. lesser "sensitivity" => Faster lookup .. perhaps my logic 
is wrong?


Hm.. or will the knngist stuff help me here by selecting the best using
pentuples from the beginning?

The above comment is actually general to pg_trgm and not to the wildcard 
search

patch above.

Jesper
--
Jesper


--
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] wildcard search support for pg_trgm

[Alexander Korotkov]

Hi!

On Mon, Jan 24, 2011 at 3:07 AM, Jan Urbański  wrote:

> I see two issues with this patch. First of them is the resulting index
> size. I created a table with 5 copies of
> /usr/share/dict/american-english in it and a gin index on it, using
> gin_trgm_ops. The results were:
>
>  * relation size: 18MB
>  * index size: 109 MB
>
> while without the patch the GIN index was 43 MB. I'm not really sure
> *why* this happens, as it's not obvious from reading the patch what
> exactly is this extra data that gets stored in the index, making it more
> than double its size.
>
Do you sure that you did comparison correctly? The sequence of index
building and data insertion does matter. I tried to build gin index on  5
copies of /usr/share/dict/american-english with patch and got 43 MB index
size.


> That leads me to the second issue. The pg_trgm code is already woefully
> uncommented, and after spending quite some time reading it back and
> forth I have to admit that I don't really understand what the code does
> in the first place, and so I don't understand what does that patch
> change. I read all the changes in detail and I could't find any obvious
> mistakes like reading over array boundaries or dereferencing
> uninitialized pointers, but I can't tell if the patch is correct
> semantically. All test cases I threw at it work, though.
>
I'll try to write sufficient comment and send new revision of patch.


> This patch changes the names and signatures of some support functions
> for GIN, and I'm not sure how that affects binary compatibility and
> pg_upgrade. I tried to create an index with the vanilla source, and then
> recompile pg_trgm and reindex the table, but it still was not using the
> index. I think it's because it's missing entries in the catalogs about
> the index supporting the like strategy. How should this be handled?
>
This patch don't alters structure of index. It only adds strategies for
index scan. In order update this index one should recreate operator class
(it will require to drop index). It can be done by
sequential uninstall_pg_trgm.sql and pg_trgm.sql. After that new index can
be created and it will support like strategy. Although actually there is no
need of index recreation, I don't see easier way to do this.


With best regards,
Alexander Korotkov.

Re: [HACKERS] Include WAL in base backup

[Magnus Hagander]

On Mon, Jan 24, 2011 at 09:03, Fujii Masao  wrote:
> On Mon, Jan 24, 2011 at 4:47 PM, Magnus Hagander  wrote:
>> On Mon, Jan 24, 2011 at 08:45, Fujii Masao  wrote:
>>> On Fri, Jan 21, 2011 at 12:28 AM, Tom Lane  wrote:
 Magnus Hagander  writes:
>> - Why not initialize logid and logseg like so?:
>>
>>        int logid = startptr.xlogid;
>>        int logseg = startptr.xrecoff / XLogSegSize;
>>
>>  Then use those in your elog?  Seems cleaner to me.

> Hmm. Yes. Agreed.

 Marginal complaint here: int is the wrong type, I'm pretty sure.
>>>
>>> And, we should use XLByteToPrevSeg here instead of just =, I think.
>>
>> Not XLByteToSeg?
>
> Checking... yeah, you are right. We should use XLByteToSeg since
> the REDO starting WAL record doesn't exist in the previous WAL
> segment when the REDO starting location is a boundary byte.

Here's an updated version of the patch:

* rebased on the current git master (including changing the switch
from -w to -x since -w was used now)
* includes some documentation
* use XLogByteToSeg and uint32 as mentioned here
* refactored to remove SendBackupDirectory(), removing the ugly closetar boolean

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 73f26b4..c257413 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1460,7 +1460,7 @@ The commands accepted in walsender mode are:
   
 
   
-BASE_BACKUP [LABEL 'label'] [PROGRESS] [FAST]
+BASE_BACKUP [LABEL 'label'] [PROGRESS] [FAST] [WAL]
 
  
   Instructs the server to start streaming a base backup.
@@ -1505,6 +1505,18 @@ The commands accepted in walsender mode are:
  
 

+
+   
+WAL
+
+ 
+  Include the necessary WAL segments in the backup. This will include
+  all the files between start and stop backup in the
+  pg_xlog directory of the base directory tar
+  file.
+ 
+
+   
   
  
  
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 321c8ca..60ffa3c 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -145,6 +145,31 @@ PostgreSQL documentation
  
 
  
+  -x
+  --xlog
+  
+   
+Includes the required transaction log files (WAL files) in the
+backup. This will include all transaction logs generated during
+the backup. If this option is specified, it is possible to start
+a postmaster directly in the extracted directory without the need
+to consult the log archive, thus making this a completely standalone
+backup.
+   
+   
+
+ The transaction log files are collected at the end of the backup.
+ Therefore, it is necessary for the
+  parameter to be set high
+ enough that the log is not removed before the end of the backup.
+ If the log has been rotated when it's time to transfer it, the
+ backup will fail and be unusable.
+
+   
+  
+ 
+
+ 
   -Z level
   --compress=level
   
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 73edcf2..9bffe17 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -37,6 +37,7 @@ typedef struct
 	const char *label;
 	bool		progress;
 	bool		fastcheckpoint;
+	bool		includewal;
 }	basebackup_options;
 
 
@@ -46,7 +47,6 @@ static void _tarWriteHeader(char *filename, char *linktarget,
 struct stat * statbuf);
 static void send_int8_string(StringInfoData *buf, int64 intval);
 static void SendBackupHeader(List *tablespaces);
-static void SendBackupDirectory(char *location, char *spcoid);
 static void base_backup_cleanup(int code, Datum arg);
 static void perform_base_backup(basebackup_options *opt, DIR *tblspcdir);
 static void parse_basebackup_options(List *options, basebackup_options *opt);
@@ -78,7 +78,10 @@ base_backup_cleanup(int code, Datum arg)
 static void
 perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
 {
-	do_pg_start_backup(opt->label, opt->fastcheckpoint);
+	XLogRecPtr	startptr;
+	XLogRecPtr	endptr;
+
+	startptr = do_pg_start_backup(opt->label, opt->fastcheckpoint);
 
 	PG_ENSURE_ERROR_CLEANUP(base_backup_cleanup, (Datum) 0);
 	{
@@ -87,12 +90,6 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
 		struct dirent *de;
 		tablespaceinfo *ti;
 
-
-		/* Add a node for the base directory */
-		ti = palloc0(sizeof(tablespaceinfo));
-		ti->size = opt->progress ? sendDir(".", 1, true) : -1;
-		tablespaces = lappend(tablespaces, ti);
-
 		/* Collect information about all tablespaces */
 		while ((de = ReadDir(tblspcdir, "pg_tblspc")) != NULL)
 		{
@@ -120,6 +117,10 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
 		

Re: [HACKERS] add __attribute__((noreturn)) to suppress a waring

[Tom Lane]

Heikki Linnakangas  writes:
> On 24.01.2011 03:42, Itagaki Takahiro wrote:
>> To suppress it, I'm thinking to add noreturn to die_horribly().
>> Any objections?  Another solution might be adding a dummy assignment
>> after calls of die_horribly().

> I added a dummy assignment, that's how we've handled this before in 
> pg_dump.

We generally do *not* want to depend on noreturn, because it will not
suppress similar warnings in non-gcc compilers.  Just add a dummy
initialization, or reorganize the code to avoid the problem.

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] gist README

[Vangelis Katsikaros]

On 01/24/2011 02:25 PM, Vangelis Katsikaros wrote:

[skip]


Oh sorry for the double posting. Thought I canceled it, my bad...

Vangelis

--
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] gist README

[Robert Haas]

On Mon, Jan 24, 2011 at 6:58 AM, Vangelis Katsikaros
 wrote:
> I see that the gist README file (src/backend/access/gist/README) hasn't seen
> any major content updates, since its initial creation, apart from gist
> insertion recovery issues [1].
>
> I would like to ask if there are any major differences between the README
> file and postrges' code, and if the README file has the essence of what is
> happening in the gist code?
>
> For example in commit (1f7ef548ec2e594fa8766781c490fb5b998ea46b) [2],
> another algorithm for splitting was implemented, but the paper isn't
> mentioned in the README file. The reason I am asking is that I am interested
> in gists (for a university project) and I would like to see how they were
> implemented in postgres.

We usually try to keep our README files up to date, but the GIST code
is more poorly commented than most of our code base, so it's certainly
possible that some updates are needed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Is there a way to build PostgreSQL client libraries with MinGW

[Xiaobo Gu]

Hi,
According to http://www.pgbuildfarm.org/cgi-bin/show_status.pl, only
GCC 3.4.2 and 4.5.0 have successfully build PostgreSQL,and only under
32 bit Windows environment, and I guess from
http://archives.postgresql.org/pgsql-hackers/2010-12/msg02073.php that
you mean we should only the above two versions of GCC to build
PostgreSQL, but my questions are:
1. I have 64bit GCC 4.5.0 installed before installing the lasted
MinGW, but after that the gcc version became to 3.4.4, which is not
support, how to change the GCC other versions than the one shipped
with MSYS.

2.You have only test 3.4.2 and 4.5.0 for 32 bit environments, do the
64 bit versions of GCC work under 64bit Windows?


Xiaobo Gu

On Mon, Jan 24, 2011 at 5:19 PM, Andrew Dunstan  wrote:
>
>
> On 01/23/2011 11:11 PM, Xiaobo Gu wrote:
>>
>> Yes, I want it working on 64 bit Windows Server 2003 R2 and 64 bit
>> Windows 7 home basic.
>> Which version of 32bit MinGW  do you use, I use the one shipped with
>> Rtools212.exe which is downloaded from
>> http://www.murdoch-sutherland.com/Rtools, and there is no chmod.
>>
>
> Please avoid top-answering on this list. I use the Mingw project's published
> files: 
>
> cheers
>
> andrew
>

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] gist README

[Vangelis Katsikaros]

Hello

I see that the gist README file (src/backend/access/gist/README) hasn't 
seen any major content updates, since its initial creation, apart from 
gist insertion recovery issues [1].


I would like to ask if there are any major differences between the 
README file and postrges' code, and if the README file has the essence 
of what is happening in the gist code?


For example in commit (1f7ef548ec2e594fa8766781c490fb5b998ea46b) [2], 
another algorithm for splitting was implemented, but the paper isn't 
mentioned in the README file. The reason I am asking is that I am 
interested in gists (for a university project) and I would like to see 
how they were implemented in postgres.


Regards
Vangelis Katsikaros


[1] 
http://git.postgresql.org/gitweb?p=postgresql.git;a=history;f=src/backend/access/gist/README;h=2d78dcb0dfaf21e348bcca3da8ba3829bcac5288;hb=master


[2] 
http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=1f7ef548ec2e594fa8766781c490fb5b998ea46


--
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] review: FDW API

[Robert Haas]

On Mon, Jan 24, 2011 at 8:08 AM, Heikki Linnakangas
 wrote:
> * Is there any point in allowing a FDW without a handler? It's totally
> useless, isn't it? We had the CREATE FOREIGN DATA WRAPPER syntax in previous
> versions, and it allowed it, but it has always been totally useless so I
> don't think we need to worry much about backwards-compatibility here.

Aren't things like dblink using this in its existing form?

> * Is there any use case for changing the handler or validator function of an
> existign FDW with ALTER? To me it just seems like an unnecessary
> complication.

+1.

> * IMHO the "FDW-info" should always be displayed, without VERBOSE. In my
> experience with another DBMS that had this feature, the SQL being sent to
> the remote server was almost always the key piece of information that I was
> looking for in the query plans.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] review: FDW API

[Heikki Linnakangas]

On 21.01.2011 17:57, Robert Haas wrote:

On Fri, Jan 21, 2011 at 10:17 AM, Heikki Linnakangas
  wrote:

On 18.01.2011 17:26, Shigeru HANADA wrote:


1) 20110118-no_fdw_perm_check.patch - This patch is not included in
last post.  This had been proposed on 2011-01-05 first, but maybe has
not been reviewd yet.  I re-propose this patch for SQL standard
conformance.  This patch removes permission check that requires USAGE
on the foreign-data wrapper at CREATE FOREIGN TABLE.
Please see original post for details.

http://archives.postgresql.org/message-id/20110105145206.30fd.69899...@metrosystems.co.jp


Committed this part.


How much review have you done of parts (3) and (4)?  The key issue for
all of the FDW work in progress seems to be what the handler API is
going to look like, and so once we get that committed it will unblock
a lot of other things.


I've gone through the code in a bit more detail now. I did a bunch of 
cosmetic changes along the way, patch attached. I also added a few 
paragraphs in the docs. We need more extensive documentation, but this 
at least marks the places where I think the docs need to go.


Comments:

* How can a FDW fetch only the columns required by the scan? The file 
FDW has to read the whole file anyhow, but it could perhaps skip calling 
the input function for unnecessary columns. But more importantly, with 
something like the postgresql_fdw you don't want to fetch any extra 
columns across the wire. I gather the way to do it is to copy 
RelOptInfo->attr_needed to private storage at plan stage, and fill the 
not-needed attributes with NULLs in Iterate. That gets a bit awkward, 
you need to transform attr_needed to something that can be copied for 
starters. Or is that information somehow available at execution phase 
otherwise?


* I think we need something in RelOptInfo to mark foreign tables. At the 
moment, you need to call IsForeignTable() which does a catalog lookup. 
Maybe a new RTEKind, or a boolean flag.


* Can/should we make ReScan optional? Could the executor just do 
EndScan+BeginScan if there's no ReScan function?


* Is there any point in allowing a FDW without a handler? It's totally 
useless, isn't it? We had the CREATE FOREIGN DATA WRAPPER syntax in 
previous versions, and it allowed it, but it has always been totally 
useless so I don't think we need to worry much about 
backwards-compatibility here.


* Is there any use case for changing the handler or validator function 
of an existign FDW with ALTER? To me it just seems like an unnecessary 
complication.


* IMHO the "FDW-info" should always be displayed, without VERBOSE. In my 
experience with another DBMS that had this feature, the SQL being sent 
to the remote server was almost always the key piece of information that 
I was looking for in the query plans.


* this check in expand_inherited_rtentry seems misplaced:


/*
 * SELECT FOR UPDATE/SHARE is not allowed on foreign tables 
because
 * they are read-only.
 */
if (newrelation->rd_rel->relkind == RELKIND_FOREIGN_TABLE &&
lockmode != AccessShareLock)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 errmsg("SELECT FOR UPDATE/SHARE is not 
allowed with foreign tables")));


I don't understand why we'd need to do that for inherited tables in 
particular. And it's not working for regular non-inherited foreign tables:


postgres=# SELECT * FROM filetbl2 FOR UPDATE;
ERROR:  could not open file "base/11933/16397": No such file or directory

* Need to document how the FDW interacts with transaction 
commit/rollback. In particular, I believe EndScan is never called if the 
transaction is aborted. That needs to be noted explicitly, and need to 
suggest how to clean up any external resources in that case. For 
example, postgresql_fdw will need to close any open cursors or result sets.


In general, I think the design is sound. What we need is more 
documentation. It'd also be nice to see the postgresql_fdw brought back 
to shape so that it works against this latest version of the api patch.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index a65b4bc..06a82a4 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2986,6 +2986,41 @@ ANALYZE measurement;
   
  
 
+ 
+  Foreign Data
+   
+foreign data
+   
+   
+PostgreSQL implements parts of the SQL/MED
+specification, which allows you to access external data that resides
+outside PostgreSQL, using normal SQL queries.
+   
+
+   
+Foreign data is accessed with help from a
+foreign data wrapper. A foreign data wrapper is a
+library that can communicate with an external data source, hiding the
+details of connecting to the data source and fetching data from it. There
+ar

[HACKERS] gist README

[Vangelis Katsikaros]

Hello

I see that the gist README file (src/backend/access/gist/README) hasn't 
seen any major content updates, since its initial creation, apart from 
gist insertion recovery issues [1].


I would like to ask if there are any major differences between the 
README file and postrges' code, and if the README file has the essence 
of what is happening in the gist code?


For example in commit (1f7ef548ec2e594fa8766781c490fb5b998ea46b) [2], 
another algorithm for splitting was implemented, but the paper isn't 
mentioned in the README file. The reason I am asking is that I am 
interested in gists (for a university project) and I would like to see 
how they were implemented in postgres.


Regards
Vangelis Katsikaros


[1] 
http://git.postgresql.org/gitweb?p=postgresql.git;a=history;f=src/backend/access/gist/README;h=2d78dcb0dfaf21e348bcca3da8ba3829bcac5288;hb=master


[2] 
http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=1f7ef548ec2e594fa8766781c490fb5b998ea46


--
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] multiset patch review

[Itagaki Takahiro]

On Mon, Jan 24, 2011 at 20:49, Robert Haas  wrote:
> I notice that this is adding keywords and syntax support for what is
> basically a PostgreSQL extension (since we certainly can't possibly be
> following the SQL standards given that we're not implementing a new
> datatype.  Is that really a good idea?

As I wrote here,
http://archives.postgresql.org/pgsql-hackers/2011-01/msg00829.php
I think we can follow the SQL standard incrementally because we
have function overloads.

One exception is the result type of collect() aggregate function.
It returns an array for now, but will return a multiset when we
support true multiset data type.

-- 
Itagaki Takahiro

-- 
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] multiset patch review

[Robert Haas]

On Mon, Jan 24, 2011 at 2:45 AM, Itagaki Takahiro
 wrote:
> [ latest patch ]

I notice that this is adding keywords and syntax support for what is
basically a PostgreSQL extension (since we certainly can't possibly be
following the SQL standards given that we're not implementing a new
datatype.  Is that really a good idea?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] REVIEW: WIP: plpgsql - foreach in

[Cédric Villemain]

2011/1/24 Pavel Stehule :
> Hello
>
>>
>>
>>> Other comments- I don't like using 'i' and 'j', you really should use
>>> better variable names, especially in large loops which contain other
>>> loops.  I'd also suggest changing the outer loop to be equivilant to the
>>> number of iterations that will be done instead of the number of items
>>> and then to *not* update 'i' inside the inner-loop.  That structure is
>>> really just confusing, imv (I certainly didn't entirely follow what was
>>> happening there the first time I read it).  Isn't there a function you
>>> could use to pull out the array slice you need on each iteration through
>>> the array?
>
> I looked on code again. There are a few results:
>
> I'll change identifiers 'i' and 'j' with any, that you send. It's
> usual identifiers for nested loops and in this case they has really
> well known semantic - it's subscript of array.
>
> But others changes are more difficult
>
> we have to iterate over array's items because it allow seq. access to
> array's data. I need a global index for function "array_get_isnull". I
> can't to use a buildin functions like array_slize_size or
> array_get_slice, because there is high overhead of array_seek
> function. I redesigned the initial part of main cycle, but code is
> little bit longer :(, but I hope, it is more readable.

btw, having array_get_isnul accessible from contrib code is nice (I
used to copy/paste it for my own needs)

>
> Regards
>
> Pavel
>
>
>>>
>>
>> I don't know a better short index identifiers than I used. But I am
>> not against to change.
>>
>> I'll try to redesign main cycle.
>>
>> Regards
>>
>> Pavel Stehule
>>
>>
>>
>>>        Thanks,
>>>
>>>                Stephen
>>>
>>> -BEGIN PGP SIGNATURE-
>>> Version: GnuPG v1.4.10 (GNU/Linux)
>>>
>>> iEYEARECAAYFAk086MEACgkQrzgMPqB3kigCzQCffx0iVSMjU2UbOgAOaY/MvtOp
>>> iKsAnA5tdhKxTssdXJ+Rda4qkhNVm26g
>>> =Yn5O
>>> -END PGP SIGNATURE-
>>>
>>>
>>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>



-- 
Cédric Villemain               2ndQuadrant
http://2ndQuadrant.fr/     PostgreSQL : Expertise, Formation et 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] REVIEW: WIP: plpgsql - foreach in

[Pavel Stehule]

Hello

>
>
>> Other comments- I don't like using 'i' and 'j', you really should use
>> better variable names, especially in large loops which contain other
>> loops.  I'd also suggest changing the outer loop to be equivilant to the
>> number of iterations that will be done instead of the number of items
>> and then to *not* update 'i' inside the inner-loop.  That structure is
>> really just confusing, imv (I certainly didn't entirely follow what was
>> happening there the first time I read it).  Isn't there a function you
>> could use to pull out the array slice you need on each iteration through
>> the array?

I looked on code again. There are a few results:

I'll change identifiers 'i' and 'j' with any, that you send. It's
usual identifiers for nested loops and in this case they has really
well known semantic - it's subscript of array.

But others changes are more difficult

we have to iterate over array's items because it allow seq. access to
array's data. I need a global index for function "array_get_isnull". I
can't to use a buildin functions like array_slize_size or
array_get_slice, because there is high overhead of array_seek
function. I redesigned the initial part of main cycle, but code is
little bit longer :(, but I hope, it is more readable.

Regards

Pavel


>>
>
> I don't know a better short index identifiers than I used. But I am
> not against to change.
>
> I'll try to redesign main cycle.
>
> Regards
>
> Pavel Stehule
>
>
>
>>        Thanks,
>>
>>                Stephen
>>
>> -BEGIN PGP SIGNATURE-
>> Version: GnuPG v1.4.10 (GNU/Linux)
>>
>> iEYEARECAAYFAk086MEACgkQrzgMPqB3kigCzQCffx0iVSMjU2UbOgAOaY/MvtOp
>> iKsAnA5tdhKxTssdXJ+Rda4qkhNVm26g
>> =Yn5O
>> -END PGP SIGNATURE-
>>
>>
>
*** ./gram.y.orig	2011-01-16 14:18:59.0 +0100
--- ./gram.y	2011-01-21 21:35:21.0 +0100
***
*** 21,26 
--- 21,27 
  #include "parser/parse_type.h"
  #include "parser/scanner.h"
  #include "parser/scansup.h"
+ #include "utils/array.h"
  
  
  /* Location tracking support --- simpler than bison's default */
***
*** 134,139 
--- 135,141 
  			PLpgSQL_datum   *scalar;
  			PLpgSQL_rec *rec;
  			PLpgSQL_row *row;
+ 			int	slice;
  		}		forvariable;
  		struct
  		{
***
*** 178,184 
  %type 	assign_var
  %type 		cursor_variable
  %type 	decl_cursor_arg
! %type 	for_variable
  %type 	for_control
  
  %type 		any_identifier opt_block_label opt_label
--- 180,186 
  %type 	assign_var
  %type 		cursor_variable
  %type 	decl_cursor_arg
! %type 	for_variable foreach_control
  %type 	for_control
  
  %type 		any_identifier opt_block_label opt_label
***
*** 190,196 
  %type 	stmt_return stmt_raise stmt_execsql
  %type 	stmt_dynexecute stmt_for stmt_perform stmt_getdiag
  %type 	stmt_open stmt_fetch stmt_move stmt_close stmt_null
! %type 	stmt_case
  
  %type 	proc_exceptions
  %type  exception_sect
--- 192,198 
  %type 	stmt_return stmt_raise stmt_execsql
  %type 	stmt_dynexecute stmt_for stmt_perform stmt_getdiag
  %type 	stmt_open stmt_fetch stmt_move stmt_close stmt_null
! %type 	stmt_case stmt_foreach_a
  
  %type 	proc_exceptions
  %type  exception_sect
***
*** 264,269 
--- 266,272 
  %token 	K_FETCH
  %token 	K_FIRST
  %token 	K_FOR
+ %token 	K_FOREACH
  %token 	K_FORWARD
  %token 	K_FROM
  %token 	K_GET
***
*** 298,303 
--- 301,307 
  %token 	K_ROWTYPE
  %token 	K_ROW_COUNT
  %token 	K_SCROLL
+ %token 	K_SLICE
  %token 	K_SQLSTATE
  %token 	K_STRICT
  %token 	K_THEN
***
*** 739,744 
--- 743,750 
  		{ $$ = $1; }
  | stmt_for
  		{ $$ = $1; }
+ | stmt_foreach_a
+ 		{ $$ = $1; }
  | stmt_exit
  		{ $$ = $1; }
  | stmt_return
***
*** 1386,1391 
--- 1392,1455 
  	}
  ;
  
+ stmt_foreach_a		: opt_block_label K_FOREACH foreach_control K_IN expr_until_loop loop_body
+ 	{
+ 		PLpgSQL_stmt_foreach_a *new = palloc0(sizeof(PLpgSQL_stmt_foreach_a));
+ 		new->cmd_type = PLPGSQL_STMT_FOREACH_A;
+ 		new->lineno = plpgsql_location_to_lineno(@2);
+ 		new->label = $1;
+ 		new->expr = $5;
+ 		new->slice = $3.slice;
+ 		new->body = $6.stmts;
+ 
+ 		if ($3.rec)
+ 		{
+ 			new->rec = $3.rec;
+ 			check_assignable((PLpgSQL_datum *) new->rec, @3);
+ 		}
+ 		else if ($3.row)
+ 		{
+ 			new->row = $3.row;
+ 			check_assignable((PLpgSQL_datum *) new->row, @3);
+ 		}
+ 		else if ($3.scalar)
+ 		{
+ 			Assert($3.scalar->dtype == PLPGSQL_DTYPE_VAR);
+ 			new->var = (PLpgSQL_var *) $3.scalar;
+ 			check_assignable($3.scalar, @3);
+ 
+ 		}
+ 		else
+ 		{
+ 			ereport(ERROR,
+ 	(errcode(ERRCODE_SYNTAX_ERROR),
+ 	 errmsg("loop variable of loop over array must be a record, row variable, scalar variable or list of scalar variables"),
+ 			 parser_errposition(@3)));
+ 		}
+ 
+ 		chec

Re: [HACKERS] Add support for logging the current role

[Itagaki Takahiro]

On Wed, Jan 19, 2011 at 14:36, Stephen Frost  wrote:
> Alright, here's the latest on this patch.  I've added a log_csv_fields
> GUC along with the associated magic to make it work (at least for me).
> Also added 'role_name' and '%U' options.  Requires postmaster restart to
> change, didn't include any 'short-cut' field options, though I don't
> think it'd be hard to do if we can decide on it.  Default remains the
> same as what was in 9.0.

Hi, I reviewed log_csv_options.patch. It roughly looks good,
but I'd like to discuss about the design in some points.

 Features 
The patch adds "log_csv_fields" GUC variable. It allows to customize
columns in csvlog. The default setting is what we were writing 9.0 or
earlier versions.

It also add "role_name" for log_csv_fields and "%U" for log_line_prefix
to record role names. They are the name set by SET ROLE. OTOH, user_name
and %u shows authorization user names.

 Discussions 
* How about "csvlog_fields" rather than "log_csv_fields"?
  Since we have variables with syslog_ prefix, csvlog_ prefix
  seems to be better.

* We use % syntax to specify fields in logs for log_line_prefix,
  but will use long field names for log_csv_fields. Do you have any
  better idea to share the names in both options?  At least I want to
  share the short description for them in postgresql.conf.

* log_csv_fields's GUC context is PGC_POSTMASTER. Is it by design?
  PGC_SIGHUP would be more consistent compared with log_line_prefix.
  However, the csv format will not be valid because the column
  definitions will be changed in the middle of file.

* "none" is not so useful for the initial "role_name" field.
  Should we use user_name instead of the empty role_name?

 Comments for codes 
Some of the items are trivial, though.

* What objects do you want to allocate in TopMemoryContext in
  assign_log_csv_fields() ? AFAICS, we don't have to keep rawstring
  and column_list in long-term context. Or, if you need TopMemoryContext,
  those variables should be pfree'ed at the end of function.

* appendStringInfoChar() calls in write_csvlog() seem to be wrong
  when the last field is not application_name.

* Docs need more cross-reference hyper-links for "see also" items.

* Docs need some tags for itemized elements or pre-formatted codes.
  They looks itemized in the sgml files, but will be flattened in
  complied HTML files.

* A declaration of assign_log_csv_fields() at the top of elog.c
  needs "extern".
* There is a duplicated declaration for build_default_csvlog_list().
* list_free() is NIL-safe. You don't have to check whether the list
  is NIL before call the function.

-- 
Itagaki Takahiro

-- 
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] Bug? Unexpected argument handling in pl-python variadic argument function

[Jan Urbański]

On 24/01/11 02:01, Nate C wrote:
> Unexpected argument handling in pl/python variadic argument function
> 
> create or replace function variadic_sql
>   (template text, variadic args text[], out text)
>   language sql as
>   $$
>   select $1 || '  --  ' || $2::text
>   $$;
> 
> create or replace function variadic_python
>   (template text, variadic args text[], out text)
>   language plpythonu as
>   $$
>   return template + '  --  ' + str(args)
>   $$;
> 
> 
> -- expected
> select variadic_sql('{foo}{bar}', '1', '2');
> variadic_sql
> 
>  {foo}{bar}  --  {1,2}
> 
> 
> -- first scalar arg also in the variadic args
> select variadic_python('{foo}{bar}', '1', '2');
> 
> 
>variadic_python
> --
>  {foo}{bar}  --  ['{foo}{bar}', ['1', '2']]

You've chosen an unfortunate name for the variadic argument: see
http://www.postgresql.org/docs/current/static/plpython-funcs.html

PL/Python functions automatically have a global "args" variable that is
a list of all the arguments. That's necessary because you can have
functions that don't assign names to variables, like

create function noname_concat(text, text) returns text language
plpythonu as $$
  return args[0] + args[1]
$$;

Perhaps we should throw an error if you try to define a function with an
explicit "args" variable? It'd be a backwards-compatibility problem, but
then again these functions are probably already broken for people.


> I could not find very much documentation and only this on the lists:
> 
> from Jan Urbański on his planned improvements for pl/python:
> http://archives.postgresql.org/pgsql-hackers/2010-12/msg00551.php
> 
>> * variadic argument handling (is this possible for a non-plpgsql pl?)

Yes, I thought it would require some work but it turned out that the
generic mechanism already worked for variadic arguments, so there's no
need for specific PL/Python code to support them.

Cheers,
Jan

-- 
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] Is there a way to build PostgreSQL client libraries with MinGW

[Andrew Dunstan]

On 01/23/2011 11:11 PM, Xiaobo Gu wrote:

Yes, I want it working on 64 bit Windows Server 2003 R2 and 64 bit
Windows 7 home basic.
Which version of 32bit MinGW  do you use, I use the one shipped with
Rtools212.exe which is downloaded from
http://www.murdoch-sutherland.com/Rtools, and there is no chmod.



Please avoid top-answering on this list. I use the Mingw project's 
published files: 


cheers

andrew

--
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: Add PGXS support to hstore's Makefile (trivial)

[Dimitri Fontaine]

Robert Haas  writes:

> On Sun, Jan 23, 2011 at 9:20 PM, Joseph Adams
>  wrote:
>> The attached patch changes hstore's Makefile so it is like the others.
>
> Thanks, committed.

For the record, I smell bitrot here:

  http://archives.postgresql.org/message-id/m21v7jpqe6@2ndquadrant.fr

I had included this patch into my extension branch and patch, so this is
a good excuse for me to produce a v27.  Will do in a separate thread.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et 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] REVIEW: WIP: plpgsql - foreach in

[Pavel Stehule]

2011/1/24 Stephen Frost :
> Pavel,
>
> * Pavel Stehule (pavel.steh...@gmail.com) wrote:
>> I merge your changes and little enhanced comments.
>
> Thanks.  Reviewing this further-
>
> Why are you using 'FOREACH' here instead of just making it another
> variation of 'FOR'?  What is 'FOUND' set to following this?  I realize
> that might make the code easier, but it really doesn't seem to make much
> sense to me from a usability point of view.

FOR keyword - please, look on thread about my proposal FOR-IN-ARRAY

I work with FOUND variable, because I like a consistent behave with
FOR statement. When FOUND is true after cycle, you are sure, so there
was a minimally one iteration.

>
> There also appears to be some extra whitespace changes that aren't
> necessary and a number of places where you don't follow the indentation
> conventions (eg: variable definitions in exec_stmt_foreach_a()).

I am really not sure about correct indentation of variables :(, if you
know a correct number of spaces, please, fix it.

>
> I'm still not really thrilled with how the checking for scalar vs.
> array, etc, is handled.  Additionally, what is this? :
>
>        else if (stmt->row != NULL)
>        {
>            ctrl_var = estate->datums[stmt->row->dno];
>        }
>        else
>        {
>            ctrl_var = estate->datums[stmt->rec->dno];
>        }
>


PLpgSQL distinct between vars, row and record values. These structures
can be different and information about variable's offset in frame can
be on different position in structure. This IF means:

1) get offset of target variable
2) get addr, where is target variable saved in current frame

one note: a scalar or array can be saved on var type, only scalar can
be used on row or record type. This is often used pattern - you can
see it more time in executor.


> Other comments- I don't like using 'i' and 'j', you really should use
> better variable names, especially in large loops which contain other
> loops.  I'd also suggest changing the outer loop to be equivilant to the
> number of iterations that will be done instead of the number of items
> and then to *not* update 'i' inside the inner-loop.  That structure is
> really just confusing, imv (I certainly didn't entirely follow what was
> happening there the first time I read it).  Isn't there a function you
> could use to pull out the array slice you need on each iteration through
> the array?
>

I don't know a better short index identifiers than I used. But I am
not against to change.

I'll try to redesign main cycle.

Regards

Pavel Stehule



>        Thanks,
>
>                Stephen
>
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk086MEACgkQrzgMPqB3kigCzQCffx0iVSMjU2UbOgAOaY/MvtOp
> iKsAnA5tdhKxTssdXJ+Rda4qkhNVm26g
> =Yn5O
> -END PGP SIGNATURE-
>
>

-- 
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] review: FDW API

[Itagaki Takahiro]

On Sat, Jan 22, 2011 at 07:20, Tom Lane  wrote:
> Heikki Linnakangas  writes:
>> * I wonder if CREATE FOREIGN DATA WRAPPER should automatically create
>> the handler function, if it doesn't exist yet.
>
> Doing that would require the equivalent of pg_pltemplate for FDWs, no?
> I think we're a long way from wanting to do that.  Also, it seems to me
> that add-on FDWs are likely to end up getting packaged as extensions,

The proposed file_fdw.sql actually creates a default FDW on installation.
So I think the installation scripts work as a template even if we don't
have FDW template catalogs.

+ /* contrib/file_fdw/file_fdw.sql.in */
+ -- create wrapper with validator and handler
+ CREATE OR REPLACE FUNCTION file_fdw_validator (text[], oid)
+ CREATE OR REPLACE FUNCTION file_fdw_handler ()
+ CREATE FOREIGN DATA WRAPPER file_fdw
+ VALIDATOR file_fdw_validator HANDLER file_fdw_handler;

-- 
Itagaki Takahiro

-- 
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] Include WAL in base backup

[Fujii Masao]

On Mon, Jan 24, 2011 at 4:47 PM, Magnus Hagander  wrote:
> On Mon, Jan 24, 2011 at 08:45, Fujii Masao  wrote:
>> On Fri, Jan 21, 2011 at 12:28 AM, Tom Lane  wrote:
>>> Magnus Hagander  writes:
> - Why not initialize logid and logseg like so?:
>
>        int logid = startptr.xlogid;
>        int logseg = startptr.xrecoff / XLogSegSize;
>
>  Then use those in your elog?  Seems cleaner to me.
>>>
 Hmm. Yes. Agreed.
>>>
>>> Marginal complaint here: int is the wrong type, I'm pretty sure.
>>
>> And, we should use XLByteToPrevSeg here instead of just =, I think.
>
> Not XLByteToSeg?

Checking... yeah, you are right. We should use XLByteToSeg since
the REDO starting WAL record doesn't exist in the previous WAL
segment when the REDO starting location is a boundary byte.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers