Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp writes:
AFAICS, the assertion is broken, but the code is correct. We just need to
adjust the expression in the assertion.
I think this is 100% wrong. Toast tables shouldn't be changing
namespace either; which means you broke something somewhere
I wrote:
Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp writes:
AFAICS, the assertion is broken, but the code is correct. We just need to
adjust the expression in the assertion.
I think this is 100% wrong. Toast tables shouldn't be changing
namespace either; which means you broke something
I wrote:
After tracing through it, the problem is that rebuild_relation() assumes
toast tables are always in PG_TOAST_NAMESPACE; which has not been true
since 8.3. CLUSTER has been renaming temp toast tables into the wrong
namespace right along. Without the assert to call attention to it,
TRAP: FailedAssertion(!(typeNamespace == typ-typnamespace), File:
pg_type.c, Line: 658)
Test case attached, repeated, consistent failure on CVS HEAD.
Crash occurs with either CLUSTER or VACUUM FULL.
Passed on without further investigation.
--
Simon Riggs www.2ndQuadrant.com
--
--
Simon Riggs si...@2ndquadrant.com wrote:
TRAP: FailedAssertion(!(typeNamespace == typ-typnamespace), File:
pg_type.c, Line: 658)
Test case attached, repeated, consistent failure on CVS HEAD.
I see the same assertion failure on 8.4.2, too.
I'll investigating it...
-- minimum reproducible
Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp wrote:
TRAP: FailedAssertion(!(typeNamespace == typ-typnamespace), File:
pg_type.c, Line: 658)
It comes from swapping toast relations:
DEBUG: typeNamespace mismatch: 99 (pg_toast) vs. 16386 (pg_toast_temp_2)
AFAICS, the assertion is
On Wed, 2010-01-06 at 14:41 +0900, Takahiro Itagaki wrote:
I just applied the patch with a few additional comments.
I just realised that this new feature *removes* any clustering that was
previously defined on a table. Many people would see that as a bug and
would say that VACUUM FULL should
Simon Riggs wrote:
On Wed, 2010-01-06 at 14:41 +0900, Takahiro Itagaki wrote:
I just applied the patch with a few additional comments.
I just realised that this new feature *removes* any clustering that was
previously defined on a table.
Hmm, that's an overstatement. If the table was in
On Wed, 2010-01-06 at 14:41 +0900, Takahiro Itagaki wrote:
Simon Riggs si...@2ndquadrant.com wrote:
1. Commit your patch, as-is (you/me)
I assume this is OK with you now?
I just applied the patch with a few additional comments.
Also, I adjusted some messages for vacuumdb to be
On ons, 2010-01-06 at 14:41 +0900, Takahiro Itagaki wrote:
Simon Riggs si...@2ndquadrant.com wrote:
1. Commit your patch, as-is (you/me)
I assume this is OK with you now?
I just applied the patch with a few additional comments.
Please clean up the compiler warnings:
cluster.c: In
On Mon, 2010-01-04 at 08:04 +, Simon Riggs wrote:
I would prefer this slightly modified version
1. Commit your patch, as-is (you/me)
I assume this is OK with you now?
--
Simon Riggs www.2ndQuadrant.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
Simon Riggs si...@2ndquadrant.com wrote:
1. Commit your patch, as-is (you/me)
I assume this is OK with you now?
I just applied the patch with a few additional comments.
Also, I adjusted some messages for vacuumdb to be look-alike
for recently-committed --only-analyze patch.
Remaining ToDo
Happy New Year,
On Mon, 2010-01-04 at 11:50 +0900, Takahiro Itagaki wrote:
Robert Haas robertmh...@gmail.com wrote:
So, what is the roadmap for getting this done? It seems like to get
rid of VFI completely, we would need to implement something like what
Tom described here:
On Mon, Jan 4, 2010 at 3:04 AM, Simon Riggs si...@2ndquadrant.com wrote:
This is a more cautious approach. Completely removing VFI in this
release is a big risk that we need not take; we have little to gain from
doing so and putting it back again will be harder. I am always keen to
push
On Mon, 2010-01-04 at 10:31 -0500, Robert Haas wrote:
On Mon, Jan 4, 2010 at 3:04 AM, Simon Riggs si...@2ndquadrant.com wrote:
This is a more cautious approach. Completely removing VFI in this
release is a big risk that we need not take; we have little to gain from
doing so and putting it
On Mon, Jan 4, 2010 at 11:57 AM, Simon Riggs si...@2ndquadrant.com wrote:
On Mon, 2010-01-04 at 10:31 -0500, Robert Haas wrote:
On Mon, Jan 4, 2010 at 3:04 AM, Simon Riggs si...@2ndquadrant.com wrote:
This is a more cautious approach. Completely removing VFI in this
release is a big risk
What I should have said, in addition: VFI will be kept as a non-default
option, in case it is required. We will document that use of VFI will
not work correctly with HS and that its use is deprecated and should be
in emergencies only in any case. I will enjoy removing VFI when that
On Mon, 2010-01-04 at 12:05 -0800, Josh Berkus wrote:
What I should have said, in addition: VFI will be kept as a non-default
option, in case it is required. We will document that use of VFI will
not work correctly with HS and that its use is deprecated and should be
in emergencies only in
On Mon, Jan 4, 2010 at 3:51 PM, Simon Riggs si...@2ndquadrant.com wrote:
On Mon, 2010-01-04 at 12:05 -0800, Josh Berkus wrote:
What I should have said, in addition: VFI will be kept as a non-default
option, in case it is required. We will document that use of VFI will
not work correctly
I think I'd vote for throwing an ERROR. By the time you see the
WARNING it may be too late. Since this is only for emergencies, the
user can shut off recovery_connections first if they really need it.
I'm with Robert on this one. If running VFI will cause unrecoverable
failure on the
On Mon, 2010-01-04 at 16:41 -0500, Robert Haas wrote:
I propose we have a WARNING if VFI being run when recovery_connections =
on, since I probably know what I'm doing if I go out of my way to use
new syntax after presumably having read the manual.
I think I'd vote for throwing an ERROR.
Robert Haas robertmh...@gmail.com writes:
On Mon, Jan 4, 2010 at 3:51 PM, Simon Riggs si...@2ndquadrant.com wrote:
Changes required to remove it are at least these places
* most of vacuum.c
* visibility checks
* heap tuple flags and xvac
* nontransactional validation
* minor points and
On Mon, Jan 4, 2010 at 8:35 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Robert Haas robertmh...@gmail.com writes:
On Mon, Jan 4, 2010 at 3:51 PM, Simon Riggs si...@2ndquadrant.com wrote:
Changes required to remove it are at least these places
* most of vacuum.c
* visibility checks
* heap tuple
Robert Haas robertmh...@gmail.com wrote:
So, what is the roadmap for getting this done? It seems like to get
rid of VFI completely, we would need to implement something like what
Tom described here:
http://archives.postgresql.org/pgsql-hackers/2009-09/msg00249.php
I'm not sure whether
On Tue, 2009-12-15 at 11:17 +0900, Takahiro Itagaki wrote:
Simon Riggs si...@2ndquadrant.com wrote:
I have enough items emerging from HS to keep me busy much longer than I
thought. I'll run with VF if that's OK, since I have some other related
changes in that area and it makes sense to
On Tue, Dec 22, 2009 at 7:29 AM, Simon Riggs si...@2ndquadrant.com wrote:
On Tue, 2009-12-22 at 19:45 +0900, Takahiro Itagaki wrote:
I used VACUUM FULL because we were discussing to drop VFI completely,
but I won't replace the behavior if hot-standby can support VFI.
HS can't support VFI
Simon Riggs si...@2ndquadrant.com wrote:
I think we
either need to implement that or document that vacuum will not skip
all-visible pages when running VACUUM FULL.
All-visible does not always mean filled enough, no? We will need to
check both visibility and fillfactor when we optimize
On Tue, 2009-12-22 at 18:11 +0900, Takahiro Itagaki wrote:
Old VACUUM FULL was substantially faster than this on tables that
had
nothing to remove.
Yeah, that's why traditional VACUUM FULL is still kept as INPLACE
vacuum.
Please can you arrange for the cluster operation to skip
On Tue, 2009-12-22 at 18:11 +0900, Takahiro Itagaki wrote:
Instead, I'd suggest to have conditional database-wide maintenance
commands, something like:
VACUUM FULL WHERE the table is fragmented
We don't have to support the feature by SQL syntax; it could be done
in client tools. How
Simon Riggs si...@2ndquadrant.com wrote:
Our perception of acceptable change is much higher than most users. If
we tell people use VACUUM FULL or vacuumdb -f, then that command
should, if possible, continue to work well across many releases.
vacuumdb in most people's minds is the command you
On Tue, 2009-12-22 at 19:45 +0900, Takahiro Itagaki wrote:
I used VACUUM FULL because we were discussing to drop VFI completely,
but I won't replace the behavior if hot-standby can support VFI.
HS can't support VFI now, by definition. We agreed to spend the time
getting rid of VFI, which
On Mon, 2009-12-07 at 16:55 +0900, Itagaki Takahiro wrote:
Tom Lane t...@sss.pgh.pa.us wrote:
You should take those out again; if I am the committer I certainly will.
Such a test will guarantee complete instability of every other
regression test, and it's not worth it.
I read the
Simon Riggs wrote:
I notice that during copy_heap_data() we make no attempt to skip pages
that are all visible according to the visibilitymap. It seems like it
would be a substantial win to copy whole blocks if all the
pre-conditions are met (I see what they are). I'm surprised to see that
On Mon, Dec 21, 2009 at 12:56 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
Simon Riggs wrote:
I notice that during copy_heap_data() we make no attempt to skip pages
that are all visible according to the visibilitymap. It seems like it
would be a substantial win to copy
Takahiro Itagaki wrote:
Here is an updated patch rebased to the latest CVS HEAD.
One remaining concern is VERBOSE. Log messages by FULL (rewrite) are less
verbose than FULL INPLACE. The same can be said for CLUSTER VERBOSE, though.
I don't have any plans to make CLUSTER more verbose in the
On 12/15/09 1:05 AM, Takahiro Itagaki wrote:
Here is an updated patch rebased to the latest CVS HEAD.
One remaining concern is VERBOSE. Log messages by FULL (rewrite) are less
verbose than FULL INPLACE. The same can be said for CLUSTER VERBOSE, though.
I don't have any plans to make CLUSTER
Alvaro Herrera alvhe...@commandprompt.com wrote:
Hmm. With this patch, if I do vacuumdb -f it will not vacuum the
special system catalogs that can only be vacuumed with INPLACE, correct?
No. It will vacuum normal tables with FULL (rewrite), and system catalogs
with FULL INPLACE. FULL vacuums
Simon Riggs si...@2ndquadrant.com wrote:
I have enough items emerging from HS to keep me busy much longer than I
thought. I'll run with VF if that's OK, since I have some other related
changes in that area and it makes sense to understand that code also, if
OK with you.
Sure. Many users
On Tue, 2009-12-15 at 11:17 +0900, Takahiro Itagaki wrote:
Simon Riggs si...@2ndquadrant.com wrote:
I have enough items emerging from HS to keep me busy much longer than I
thought. I'll run with VF if that's OK, since I have some other related
changes in that area and it makes sense to
Itagaki Takahiro itagaki.takah...@oss.ntt.co.jp writes:
I added regression tests for database-wide vacuums and check changes
of relfilenodes in those commands.
...
BTW, I needed to add ORDER BY cluase in select_views test. I didn't modify
tests in select_views at all, but database-wide vacuum
Tom Lane t...@sss.pgh.pa.us wrote:
You should take those out again; if I am the committer I certainly will.
Such a test will guarantee complete instability of every other
regression test, and it's not worth it.
I read the original comment was saying to add regression tests for
database-wide
On Fri, 2009-12-04 at 19:49 -0500, Michael Glaesemann wrote:
I tested a variety of situations during my review, and everything
worked
as I expected.
Would there be a way for you to package the scenarios you tested into
a suite?
Except for the simplest tests, they aren't easily moved
On Tue, 2009-12-01 at 01:43 -0800, Jeff Davis wrote:
Marking as ready.
You're saying its ready, yet there are 3 additional suggestions patches
attached here. Please can you resolve these and re-submit a single final
patch from author and reviewer?
I will review and eventually commit this, if
On Fri, 2009-12-04 at 09:20 +, Simon Riggs wrote:
On Tue, 2009-12-01 at 01:43 -0800, Jeff Davis wrote:
Marking as ready.
You're saying its ready, yet there are 3 additional suggestions patches
attached here. Please can you resolve these and re-submit a single final
patch from author
On Fri, 2009-12-04 at 09:56 -0800, Jeff Davis wrote:
We could put a VACUUM FULL; and a VACUUM (FULL INPLACE); somewhere,
which will cover a lot of the cases you're talking about. However, that
may be a performance consideration especially for people who develop on
laptops.
Let's check it
On Fri, 2009-12-04 at 18:36 +, Simon Riggs wrote:
Let's check it works before worrying about performance. We can take
tests out as well as add them once it becomes obvious its working.
Itagaki-san, perhaps you should add a variety of tests, and then Simon
can remove extra tests after he's
On Dec 4, 2009, at 18:07 , Jeff Davis wrote:
On Fri, 2009-12-04 at 18:36 +, Simon Riggs wrote:
Let's check it works before worrying about performance. We can take
tests out as well as add them once it becomes obvious its working.
Itagaki-san, perhaps you should add a variety of tests,
On Mon, 2009-11-30 at 14:38 +0900, Itagaki Takahiro wrote:
* VACUUM (FULL REPLACE) pg_class should be rejected, not silently
turned into VACUUM (FULL INPLACE) pg_class.
Hmmm, it requires to remember whether REPLACE is specified or not
for the non-INPLACE vacuum, but I don't want to add
Itagaki Takahiro wrote:
Done. (vacuum-full_20091130.patch)
Is this ready for a committer now? Not sure whether Jeff intends to
re-review here or not, given that the suggestions and their fixes were
pretty straightforward. It looks pretty solid at this point to me.
--
Greg Smith
On Mon, 2009-11-30 at 15:10 -0500, Greg Smith wrote:
Itagaki Takahiro wrote:
Done. (vacuum-full_20091130.patch)
Is this ready for a committer now? Not sure whether Jeff intends to
re-review here or not, given that the suggestions and their fixes were
pretty straightforward. It looks
Thanks for review!
Jeff Davis pg...@j-davis.com wrote:
* VACUUM (FULL REPLACE) pg_class should be rejected, not silently
turned into VACUUM (FULL INPLACE) pg_class.
Hmmm, it requires to remember whether REPLACE is specified or not
for the non-INPLACE vacuum, but I don't want to add
Review comments:
* I attached some documentation suggestions.
* The patch should be merged with CVS HEAD. The changes required are
minor; but notice that there is a minor style difference in the assert
in vacuum().
* vacuumdb should reject -i without -f
* The replace or inplace option is a
Here is an updated patch of rewriting vacuum based on vacuum options patch.
Documentations and vacuumdb modification (-i, --inplace) are added.
Jeff Davis pg...@j-davis.com wrote:
1. Do we want to introduce syntax for INPLACE at all, if we are
eventually going to remove the current mechanism?
Jeff Davis pg...@j-davis.com writes:
On Mon, 2009-11-16 at 13:37 +0900, Itagaki Takahiro wrote:
[ new options syntax for VACUUM ]
Great, I am marking this part ready for committer.
Applied with very minor editorialization.
regards, tom lane
--
Sent via pgsql-hackers
Jeff Davis pg...@j-davis.com wrote:
You left INPLACE in the patch
Oops, removed.
Sounds fine, but worth a mention in the documentation. Just add to the
columns part of the VACUUM page something like: If specified, implies
ANALYZE.
Added.
Other than these two minor issues, I don't see
On Mon, 2009-11-16 at 13:37 +0900, Itagaki Takahiro wrote:
Jeff Davis pg...@j-davis.com wrote:
You left INPLACE in the patch
Oops, removed.
Sounds fine, but worth a mention in the documentation. Just add to the
columns part of the VACUUM page something like: If specified, implies
On Sat, 2009-11-14 at 19:25 -0500, Tom Lane wrote:
As a rule of thumb, I'd recommend keeping as much complexity as possible
*out* of gram.y. It's best if that code just reports the facts, ie what
options the user entered. Deriving conclusions from that (like what
default behaviors should be
On Fri, 2009-11-13 at 10:47 +0900, Itagaki Takahiro wrote:
Alvaro Herrera alvhe...@commandprompt.com wrote:
BTW I think the vacstmt.options change, which seems a reasonable idea to
me, could be extracted from the patch and applied separately. That'd
reduce the size of the patch somewhat.
Jeff Davis pg...@j-davis.com writes:
I'd like to try to summarize the goals we have for VACUUM to put these
patches in perspective:
Good summary, but ...
Some other ideas also came out of the thread, like:
* Provide a way to truncate the dead space from the end of a relation in
a blocking
On Fri, 2009-11-13 at 10:47 +0900, Itagaki Takahiro wrote:
Alvaro Herrera alvhe...@commandprompt.com wrote:
BTW I think the vacstmt.options change, which seems a reasonable idea to
me, could be extracted from the patch and applied separately. That'd
reduce the size of the patch somewhat.
On Tue, 2009-10-27 at 13:55 +0900, Itagaki Takahiro wrote:
Here is a patch to support rewrite version of VACUUM FULL.
Can you please provide a patch that applies cleanly on top of the vacuum
options patch and on top of CVS HEAD (there are a couple minor conflicts
with this version). That would
Jeff Davis pg...@j-davis.com writes:
3. Some options are being set in vacuum() itself. It looks like the
options should already be set in gram.y, so should that be an Assert
instead? I think it's cleaner to set all of the options properly early
on, rather than waiting until vacuum() to
Itagaki Takahiro wrote:
Here is a patch to support rewrite version of VACUUM FULL.
It was called VACUUM REWRITE in the past disucussin,
but I choose the following syntax for now:
VACUUM ( FULL [ INPLACE | REPLACE ] ) [ table_name ]
The reason is to contrast the new REPLACE behavior
Itagaki Takahiro wrote:
We still need traditional VACUUM FULL behavior for system catalog because
we cannot change relfilenode for them. Also, VACUUM FULL REPLACE is not
always better than traditional VACUUM FULL; the new version requires
additional disk space and might be slower if we have a
BTW I think the vacstmt.options change, which seems a reasonable idea to
me, could be extracted from the patch and applied separately. That'd
reduce the size of the patch somewhat.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication,
Alvaro Herrera alvhe...@commandprompt.com wrote:
BTW I think the vacstmt.options change, which seems a reasonable idea to
me, could be extracted from the patch and applied separately. That'd
reduce the size of the patch somewhat.
It's a good idea. I separated the part into the attached
66 matches
Mail list logo
