/postgr.es/m/CAH2-Wz=ilnf+0csab37efxcgmrjo1dyjw5hmzm7tp1axg1n...@mail.gmail.com
-- scroll down to "TPC-C", which has the relevant autovacuum log
output for the orders table, covering a 24 hour period
--
Peter Geoghegan
sed on averting MultiXact
wraparound? I'm hoping that the patch that adds smarter tracking of
final relfrozenxid/relminmxid values during VACUUM makes this less of
a problem automatically.
--
Peter Geoghegan
slow the last few times I used
it, which wasn't for long enough for it to really matter. This must
have been why. I might have to rescind my recommendation of lld.
--
Peter Geoghegan
ort for a table where the failsafe kicked in).
--
Peter Geoghegan
rep` as well as more reliable given specific variations
> in output style
> depending on how the blocks are specified.
Sounds useful to me.
--
Peter Geoghegan
On Sun, Feb 20, 2022 at 12:27 PM Peter Geoghegan wrote:
> You've given me a lot of high quality feedback on all of this, which
> I'll work through soon. It's hard to get the balance right here, but
> it's made much easier by this kind of feedback.
Attached is v9
s themselves
are similar. We want options and maximum flexibility, everywhere.
> but if you are going to rescan the heap
> again next time before doing any index vacuuming then why we want to
> store them anyway.
It all depends, of course. The decision needs to be made using a cost
model. I suspect it will be necessary to try it out, and see.
--
Peter Geoghegan
_FROZEN(vacrel->rel, blkno, &vmbuffer))
vacrel->frozenskipped_pages++;
continue;
}
The fact that this is conditioned in part on "vacrel->aggressive"
concerns me here. Why should we have a special case for this, where we
condition something on aggressive-ness that isn't actually strictly
related to that? Why not just remember that the range that we're
skipping was all-frozen up-front?
That way non-aggressive VACUUMs are not unnecessarily at a
disadvantage, when it comes to being able to advance relfrozenxid.
What if we end up not incrementing vacrel->frozenskipped_pages when we
easily could have, just because this is a non-aggressive VACUUM? I
think that it's worth avoiding stuff like that whenever possible.
Maybe this particular example isn't the most important one. For
example it probably isn't as bad as the one was fixed by the
lazy_scan_noprune work. But why even take a chance? Seems easier to
remove the special case -- which is what this really is.
> FWIW, I'd really like to get rid of SKIP_PAGES_THRESHOLD. It often ends up
> causing a lot of time doing IO that we never need, completely trashing all CPU
> caches, while not actually causing decent readaead IO from what I've seen.
I am also suspicious of SKIP_PAGES_THRESHOLD. But if we want to get
rid of it, we'll need to be sensitive to how that affects relfrozenxid
advancement in non-aggressive VACUUMs IMV.
Thanks again for the review!
--
Peter Geoghegan
On Fri, Feb 25, 2022 at 2:00 PM Peter Geoghegan wrote:
> > Hm. I guess I'll have to look at the code for it. It doesn't immediately
> > "feel" quite right.
>
> I kinda think it might be. Please let me know if you see a problem
> with what I've sai
is this:
in general, there are probably quite a few opportunities for
FreezeMultiXactId() to avoid allocating new XMIDs (just to freeze
XIDs) by having the full context. And maybe by making the dialog
between lazy_scan_prune and heap_prepare_freeze_tuple a bit more
nuanced.
--
Peter Geoghegan
> It might make sense to separate the purposes of SKIP_PAGES_THRESHOLD. The
> relfrozenxid advancement doesn't benefit from visiting all-frozen pages, just
> because there are only 30 of them in a row.
Right. I imagine that SKIP_PAGES_THRESHOLD actually does help with
this, but if we actually tried we'd find a much better way.
> I wish somebody would tackle merging heap_page_prune() with
> vacuuming. Primarily so we only do a single WAL record. But also because the
> separation has caused a *lot* of complexity. I've already more projects than
> I should, otherwise I'd start on it...
That has value, but it doesn't feel as urgent.
--
Peter Geoghegan
ssions are quite possible, and
a real concern -- but regressions *like that* are unlikely. Avoiding
doing what is clearly the wrong thing just seems to work out that way,
in general.
--
Peter Geoghegan
On Fri, Feb 25, 2022 at 5:52 PM Peter Geoghegan wrote:
> There is an important practical way in which it makes sense to treat
> 0001 as separate to 0002. It is true that 0001 is independently quite
> useful. In practical terms, I'd be quite happy to just get 0001 into
> Postgres
time to further review this patch and/or commit it?
I'll definitely review it some more before too long.
--
Peter Geoghegan
g palloc0() rather than palloc()) makes
sense as a defensive measure. It depends on the specific code, of
course.
--
Peter Geoghegan
ystem to reach
xidStopLimit due to the target rel's relfrozenxid age crossing the
crucial xidStopLimit crossover point.
This patch makes this problem scenario virtually impossible. Right now
I'm only prepared to say it's very unlikely. I don't see a reason to
take any chances, tho
On Thu, Mar 17, 2022 at 6:15 AM Peter Eisentraut
wrote:
> committed, thanks
Glad that this finally happened. Thanks to everybody involved!
--
Peter Geoghegan
On Wed, Mar 9, 2022 at 4:46 PM Andres Freund wrote:
> On 2022-03-03 19:31:32 -0800, Peter Geoghegan wrote:
> > Attached is a new revision of my fix. This is more or less a
> > combination of my v4 fix from November 12 [1] and Andres'
> > already-committed fix (commit 18
y index tuples as a result of avoiding
an FPI. This second idea is also much more general than simply
avoiding FPIs in general.
--
Peter Geoghegan
ed to test 4 branches at once and to try
> to test every branch every 24 hours. Let's see how that goes.
Extravagance!
--
Peter Geoghegan
a very simple method of making the
same information more visible, that you could implement in only a few
minutes. Perhaps that was optimistic.
--
Peter Geoghegan
ing solutions that focus on limiting the
downside of not setting LP_DEAD bits, which is local information (not
system wide information) that is much easier to understand and target
in the implementation.
--
Peter Geoghegan
On Sun, Mar 13, 2022 at 9:05 PM Peter Geoghegan wrote:
> Attached is v10. While this does still include the freezing patch,
> it's not in scope for Postgres 15. As I've said, I still think that it
> makes sense to maintain the patch series with the freezing stuff,
> s
eft behind by opportunistic pruning. We don't need a
cleanup in either lazy_scan_noprune (a share lock is all we need), nor
do we even need one in lazy_vacuum_heap_page (a regular exclusive lock
is all we need).
--
Peter Geoghegan
ably 50 million XIDs behind
OldestXmin, the vacuum_freeze_min_age default). I don't see much
difference.
Anyway, this isn't important. I'll just drop the third patch.
--
Peter Geoghegan
making
relfrozenxid advancement unsafe.
It would be great if you could take a look v11-0002-*, Robert. Does it
make sense to you?
Thanks
--
Peter Geoghegan
hing.
Generational garbage collection influenced this work, too. It also
applies pragmatic assumptions about where garbage is likely to appear.
Assumptions that are based on nothing more than empirical observations
about what is likely to happen in the real world, that are validated
ct example of the kind of hardening that makes
sense to me. Clearly this kind of failure is a reasonably plausible
scenario (probably even a known real world scenario with catalog
corruption), and clearly we could do something about it that's pretty
simple and obviously low risk. It easily meets the standard that I
might apply here.
--
Peter Geoghegan
w that I'll
be *far* more likely to use it if I don't have to waste time and
energy on that aspect every single time.
--
Peter Geoghegan
lse, but for me this is only a very small
inconvenience. Whereas the convenience of not having to think about
the performance impact seems huge.
--
Peter Geoghegan
On Mon, Dec 7, 2020 at 2:05 AM Andrey Borodin wrote:
> Here's version with tests and docs. I still have no idea how to print some
> useful information about tuples keys.
I suggest calling BuildIndexValueDescription() from your own custom
debug instrumentation code.
--
Peter Geoghegan
direction of simplification,
it should not make this other important task harder.
--
Peter Geoghegan
n via the storage parameter (presumably due to the
performance trade-off somehow not seeming useful), they cannot expect
to get back to an on-disk representation without posting list tuples,
unless and until they REINDEX.
--
Peter Geoghegan
ible downside. Even still, I think that adding
new rules is subject to sharp diminishing returns. There just aren't
that many things that work like that.
--
Peter Geoghegan
ent that
*some* combination of these two ideas (or other ideas) will work very
well. Right now we don't have the appropriate general framework.
> Any thoughts?
Nothing to add to what you said, really. I agree that it makes sense
to think of all of these things at the same time.
It'll be easier to see how far these different ideas can be pushed
once a prototype is available.
> I'm writing a PoC patch so will share it.
Great! I suggest starting a new thread for that.
--
Peter Geoghegan
On Wed, Dec 9, 2020 at 5:12 PM Peter Geoghegan wrote:
> Most of the real changes in v11 (compared to v10) are in heapam.c.
> I've completely replaced the table_compute_xid_horizon_for_tuples()
> interface with a new interface that supports all existing requirements
> (from ind
orks because GetTransactionSnapshot() will already have been called
> somewhere, before EnterParallelMode()?
It seems unlikely that InitializeParallelDSM() was ever intended to be
run in a background worker.
--
Peter Geoghegan
2.
heap_compute_xid_horizon_for_tuples() is a descendant of code added by
Simon’s commit a760893d in 2010 -- pretty close to HOT’s initial
introduction. So this has been around for a long time.
--
Peter Geoghegan
0001-Instrument-heap_compute_xid_horizon_for_tuples.patch
Description: Binary data
zy vacuum to
not do what gets requested by amvacuumcleanup() when it cannot respect
the wishes of one individual indexes, for example when the
accumulation of LP_DEAD items in the heap becomes a big problem in
itself? That really could be the thing that forces full heap
vacuuming, even with several indexes.
I will need to experiment in order to improve my understanding of how
to make this cooperate with bottom-up index deletion. But that's
mostly just a question for my patch (and a relatively easy one).
--
Peter Geoghegan
e. I expect
little impact on small installations.
[1]
https://postgr.es/m/cad21aod0ske11fmw4jd4renawbmcw1wasvnwpjvw3tvqpoq...@mail.gmail.com
--
Peter Geoghegan
ept in the extreme case where the
index is *exactly* the same size as it was after the last VACUUM.
This will happen regularly with bottom-up index deletion. Maybe that
approach is a bit too conservative, though.
--
Peter Geoghegan
On Sun, Dec 27, 2020 at 11:41 PM Peter Geoghegan wrote:
> I experimented with this today, and I think that it is a good way to
> do it. I like the idea of choose_vacuum_strategy() understanding that
> heap pages that are subject to many non-HOT updates have a "natural
> extra cap
On Tue, Dec 22, 2020 at 9:52 AM Peter Geoghegan wrote:
> ISTM that heap_compute_xid_horizon_for_tuples() calculates
> latestRemovedXid for index deletion callers without sufficient care.
> The function only follows line pointer redirects, which is necessary
> but not sufficient
actually happen, but it's possible in theory,
and if it did happen it really wouldn't be so bad).
[1]
https://postgr.es/m/CAH2-Wz=rpkb5vxs7az+v8t3ve75v_dkgro+w4nwd64e9yic...@mail.gmail.com
--
Peter Geoghegan
On Mon, Dec 28, 2020 at 11:20 PM Peter Geoghegan wrote:
> > That's a very good result in terms of skipping lazy_vacuum_heap(). How
> > much the table and indexes bloated? Also, I'm curious about that which
> > tests in choose_vacuum_strategy() turned vacuum_heap
On Mon, Dec 28, 2020 at 9:49 PM Peter Geoghegan wrote:
> I would like to commit this patch to v12, the first version that did
> this process during original execution rather than in REDO routines.
> It seems worth keeping the back branches in sync here. I suspect that
> the old a
UP_FAVORABLE_STRIDE 3
>
> Adding a comment on why the number 3 is chosen would be helpful for people to
> understand the code.
Noted - I will add something about that to the next revision.
--
Peter Geoghegan
without scanning the proc array (in cases where it will still do so
today, since of course ResolveRecoveryConflictWithSnapshot() has the
obvious InvalidTransactionId fast path already).
> Is it a sane idea or I have missed something huge?
It seems like two almost distinct ideas to me. Thoug
master, so that pg_waldump will use struct field names?
It doesn't seem necessary to change the output that has spaces instead
of an underscore, or something like that. It just seems worth removing
gratuitous inconsistencies, such as this one.
--
Peter Geoghegan
big issue for me. I really find it convenient
to use "git format-patch" to produce patches for complicated long
lived projects -- having commit messages and explicit ordering of
multiple patches is really helpful. I've come to prefer unified,
perhaps as a result of using "git format-patch" so much.
--
Peter Geoghegan
s as a symbol name 84 times, across
many files in several distinct areas. Whereas remxid means nothing to
me unless I go look it up, which is not ideal.
--
Peter Geoghegan
On Sun, Jan 3, 2021 at 8:58 PM Masahiko Sawada wrote:
> On Mon, Jan 4, 2021 at 12:55 PM Peter Geoghegan wrote:
> +1 for changing heapdesc.c on master. It's not only readable but also
> consistent with other *desc showing the field named latestRemovedXid.
> For instan
On Mon, Jan 4, 2021 at 2:06 PM Peter Geoghegan wrote:
> Right. Self-consistency matters, as does consistency with the source
> code itself.
Pushed a commit that standardizes on the name latestRemovedXid within
rmgr desc output routines just now.
Thanks
--
Peter Geoghegan
duplicates in indexes".
I'll add something like that to the next revision.
> I see. Would be good to explain that pattern with multiple indexes in
> the comment.
Will do -- it is the single best example of how heap block locality
can matter with a real workload, so it makes sense to go with it in
explanatory comments.
--
Peter Geoghegan
speak of 15x.
+1. I am *strongly* opposed to enabling checksums by default for this
reason. I think that it's a total non-starter, unless and until the
overhead can be dramatically reduced. The fact that it isn't the fault
of the checksum mechanism in some abstract sense is 100% irrelevant.
--
Peter Geoghegan
d this argument baffling. Do you really believe this?
--
Peter Geoghegan
and
that it logically follows that disagreeing with you is essentially
irresponsible. This is a tactic that would be an embarrassment to a
high school debate team. It's below you.
Your argument may be logically consistent, but it's still nonsense.
--
Peter Geoghegan
appens.
We added page-level checksums in 9.3. Can you imagine a counterfactual
history in which Postgres had page checksums since the 1990s, but only
added the fsync feature in 9.3? Please answer this non-rhetorical
question.
--
Peter Geoghegan
ing "show data_checksums;" in psql in each case?
What does "show full_page_writes;" show you?
--
Peter Geoghegan
On Wed, Jan 6, 2021 at 1:08 PM Peter Geoghegan wrote:
> So you tested this using "show data_checksums;" in psql in each case?
>
> What does "show full_page_writes;" show you?
Another consideration is checkpoint_timeout and max_wal_size.
--
Peter Geoghegan
e to say on this thread. I am -1 on the current
proposal to enable page-level checksums by default. I may change my
mind on this in the future, perhaps when underlying architectural
details change, but right now this seems like a net negative.
--
Peter Geoghegan
irtying the page)
should be cheaper than dirtying a page. This thread began because I
wanted to discuss the relative cost of different kinds of I/O
operations to VACUUM, without necessarily discussing the absolute
costs/time delays.
--
Peter Geoghegan
On Wed, Jan 6, 2021 at 5:39 PM Masahiko Sawada wrote:
>
> On Mon, Dec 28, 2020 at 5:17 AM Peter Geoghegan wrote:
> >
> > Simply decreasing vacuum_cost_page_dirty seems like a low risk way of
> > making the VACUUM costing more useful within autovacuum workers.
> >
On Sat, Jan 2, 2021 at 3:22 PM Peter Geoghegan wrote:
> Of course, it's possible that the question of whether or not it's
> worth it has been misjudged for any given case. And maybe these
> particular WAL records are one such case where somebody got it wrong,
> affecting
with this
analysis, but since you only seem to want to discuss the question in a
narrow way (e.g. "I agree that improving compression performance would
be good but I don't see that as relevant to the question of what our
defaults should be"), I have to wonder if it's worth the trouble.
--
Peter Geoghegan
hould "figure out
the best strategy for handling bloat on its own". A diversity of
strategies are available, which can adapt to many different kinds of
workloads organically. That kind of approach is sometimes necessary
with a complex system IMV.
[1] https://btw.informatik.uni-rostock.de/download/tagungsband/B2-2.pdf
--
Peter Geoghegan
ode available. Maybe that will change tomorrow or next week, but as
of this moment there is simply nothing substantive to evaluate.
--
Peter Geoghegan
res development are not set in any fixed
way (except to the limited extent that you're on the hook for anything
you integrate that breaks). I myself am not convinced that this is
worth spending any time on right now, especially given the lack of
code to evaluate.
--
Peter Geoghegan
be downloaded and tested.
--
Peter Geoghegan
't
take very long for a deleted page to become recyclable -- why wait?
This idea is enabled by commit c79f6df75dd from 2018. I think it's the
next logical step.
--
Peter Geoghegan
e
specific scenario that Masahiko expressed concern about. Nor does it
seem relevant to this patch more generally.
--
Peter Geoghegan
On Fri, Feb 12, 2021 at 9:04 PM Peter Geoghegan wrote:
> On Fri, Feb 12, 2021 at 8:38 PM Masahiko Sawada wrote:
> > I agree that there already are huge problems in that case. But I think
> > we need to consider an append-only case as well; after bulk deletion
> > on an appe
() actually be
called ReadNewFullTransactionId()? Actually, the inverse approach
looks like it produces fewer inconsistencies: you could instead rename
ReadNewTransactionId() to ReadNextTransactionId().
--
Peter Geoghegan
that it's slightly strange that the 32 and 64 bit versions differ
> here. I'd vote for renaming the 32 bit version to match...
I was just going to say the same thing myself.
Please do the honors if you have time...
--
Peter Geoghegan
On Sun, Feb 14, 2021 at 4:21 PM Thomas Munro wrote:
> Done.
Thanks.
--
Peter Geoghegan
s here.
I also notice some inconsistencies in the heap_page_prune() prototype
names vs the corresponding definition names. Might be worth doing
something about in passing.
--
Peter Geoghegan
On Sat, Feb 13, 2021 at 10:47 PM Peter Geoghegan wrote:
> It will be rare. But more importantly, the fact that scenario is now
> an extreme case justifies treating it as an extreme case. We can teach
> _bt_vacuum_needs_cleanup() to recognize it as an extreme case, too. In
> particu
On Mon, Feb 15, 2021 at 5:30 PM Andres Freund wrote:
> Done. Thanks for noticing/reporting!
Great, thanks!
--
Peter Geoghegan
e units of free space
(heapam is another story). A 100% crash-safe design would naturally
shift the problem of nbtree page recycle safety from the
producer/VACUUM side, to the consumer/_bt_getbuf() side, which I agree
would be a real improvement. But these long standing FSM issues are
not going to change for Postgres 14. And so changing _bt_getbuf() to
do clever things with XIDs won't be possible for Postgres 14 IMV.
--
Peter Geoghegan
On Mon, Feb 15, 2021 at 7:26 PM Peter Geoghegan wrote:
> Actually, there is one more reason why I bring up idea 1 now: I want
> to hear your thoughts on the index AM API questions now, which idea 1
> touches on. Ideally all of the details around the index AM VACUUM APIs
> (i.e. when a
described? I assumed that that was okay in general, but I haven't
tested parallel VACUUM specifically. Will parallel VACUUM really fail
to ensure that values in bulk stats fields (like pages_deleted and
pages_free) get set correctly for amvacuumcleanup() callbacks?
--
Peter Geoghegan
On Tue, Feb 16, 2021 at 11:35 AM Peter Geoghegan wrote:
> Isn't btvacuumcleanup() (or any other amvacuumcleanup() routine)
> entitled to rely on the bulk delete stats being set in the way I've
> described? I assumed that that was okay in general, but I haven't
no updates or deletes since the last VACUUM -- that seems
like an existing problem worth fixing now. It's way too unclear that
this setting only really concerns append-only tables.
--
Peter Geoghegan
removed by..."). Same is true of the closely related
heap_prepare_freeze_tuple()/heap_tuple_needs_freeze() code.
--
Peter Geoghegan
On Mon, Feb 22, 2021 at 2:54 PM Peter Geoghegan wrote:
> Good point. I think that the structure should make the page deletion
> triggering condition have only secondary importance -- it is only
> described at all to be complete and exhaustive. The
> vacuum_cleanup_index_scale_fa
onsidered within
btvacuumcleanup()" bug. (Though I'm less confident on this second
point about a backpatchable fix.)
--
Peter Geoghegan
code
doesn't trust the statistics too much (it's okay for VACUUM VERBOSE
output only). Right now we can get a pg_class.reltuples that is
"exactly wrong" -- it would actually be a big improvement if it was
"approximately correct".
Another new concern for me (another concer
y Postgres 13 bug fix commit
48e12913). These two issues make removing
vacuum_cleanup_index_scale_factor tempting, even in the back branches
-- it might actually be the more conservative approach, at least for
Postgres 13.
--
Peter Geoghegan
age-id/flat/20130108024957.GA4751%40tornado.leadboat.com
Of course, this effort to eliminate the "tupgone =
true"/XLOG_HEAP2_CLEANUP_INFO special case didn't go anywhere at the
time.
[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=7cde6b13a9b630e2f04d91e2f17dedc2afee21c6
[2]
https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html
--
Peter Geoghegan
On Mon, Mar 1, 2021 at 1:40 PM Peter Geoghegan wrote:
> > Since it seems not a bug I personally think we don't need to do
> > anything for back branches. But if we want not to trigger an index
> > scan by vacuum_cleanup_index_scale_factor, we could change the default
&g
n only truly fix the problem in Postgres 14
anyway, and should just accept that?
OTOH scanning the indexes for no reason when
autovacuum_vacuum_insert_scale_factor triggers an autovacuum VACUUM
does seem *particularly* silly. So I don't know what to think.
--
Peter Geoghegan
On Mon, Mar 1, 2021 at 7:00 PM Peter Geoghegan wrote:
> I think that you're right. However, in practice it isn't harmful
> because has_dead_tuples is only used when "all_visible = true", and
> only to detect corruption (which should never happen). I think that it
>
intelligent about overwriting
pg_class stats for indexes?
I define "real index vacuuming" as calling any indexes ambulkdelete() routine.
2. Does anybody anticipate any other issues? Possibly an issue that
resembles this existing known VACUUM ANALYZE issue?
Thanks
--
Peter Geoghegan
updating
pg_attribute, for example (which is something that the tests do
already).
--
Peter Geoghegan
ally don't like that the new
unique checking functionality merely warns that the index *might* be
corrupt. False positives are always unacceptable within amcheck, and I
think that this is a false positive.
--
Peter Geoghegan
at the
> > time.
>
> I'll look at that thread.
I'm not sure if it's super valuable to look at the thread. But it is
reassuring to see that Noah shared the intuition that the "tupgone =
true" case was kind of bad, even back in 2013. It's one part of my
"mental map" of VACUUM.
--
Peter Geoghegan
we know.
The user should always see the unvarnished truth. pg_amcheck should
not presume to suppress errors from lower level code, except perhaps
in well-scoped special cases.
--
Peter Geoghegan
.mqmrnpkaqrdtm...@alap3.anarazel.de
--
Peter Geoghegan
On Mon, Mar 8, 2021 at 6:41 AM Ibrar Ahmed wrote:
> The patch (0001-Add-bool-column-for-LP_DEAF-flag-to-GiST-pageinspect.patch)
> does not apply successfully and has multiple hanks failed.
That's because it was committed.
--
Peter Geoghegan
On Tue, Mar 2, 2021 at 6:01 PM Peter Geoghegan wrote:
> 1. Any objections to the idea of teaching VACUUM ANALYZE to
> distinguish between the cases where VACUUM ran and performed "real
> index vacuuming", to make it more intelligent about overwriting
> pg_class stats for in
1 - 100 of 3342 matches
Mail list logo
