is useful.
> Regarding tests, I’d like to add tests to check if a vacuum with
> multiple index scans (i.g., due to small maintenance_work_mem) works
> fine. But a problem is that we need at least about 200,000 garbage
> tuples to perform index scan twice even with the minimum
> maintenance_work_mem. Which takes a time to load tuples.
Maybe that's okay. Do you notice that it takes a lot longer now? I did
try to keep the runtime down when I committed the fixup to the
parallel VACUUM related bug.
--
Peter Geoghegan
e the chances of somebody noticing a more or less
once-off, slightly wrong answer?
--
Peter Geoghegan
s
to read the heap for at least that one dead-to-all TID, discovering
that the TID is dead to its snapshot). Why wouldn't GiST need to take
the same precautions, though?
[1] https://www.postgresql.org/docs/devel/index-locking.html
--
Peter Geoghegan
g vacuumcleanup it would not become a serious
> consequence.
>
> I've attached a patch to fix it.
I pushed your fix just now.
Thanks
--
Peter Geoghegan
e more likely it is that the notable remaining problems
will be complicated and kind of weird. That's what I see.
--
Peter Geoghegan
ctable in cases where users actually run
into real problems -- cases where it actually matters.
--
Peter Geoghegan
gs would ever
> generate if it meant that they could actually find problems when and
> if they have them.
I fully agree.
--
Peter Geoghegan
in
nbtdedup.c fails when the tests are run without the fix in place, or
something like that.
--
Peter Geoghegan
r mechanism to
make it safe -- ISTM you must "logically lock" the index structure,
using ARIES/KVL style key value locks, or something along those lines.
--
Peter Geoghegan
o imagine that it's basically just a
matter of coming up with the right index AM data structure, but that's
actually just the easy part.
--
Peter Geoghegan
[1] http://vldb.org/conf/1995/P710.PDF
[2]
https://blog.timescale.com/blog/how-we-made-distinct-queries-up-to-8000x-faster-on-postgresql/
[3]
https://www.postgresql.org/message-id/flat/7f70bd5a-5d16-e05c-f0b4-2fdfc8873489%40BlueTreble.com
[4]
https://www.postgresql.org/message-id/flat/14593.1517581614%40sss.pgh.pa.us#caf373b36332f25acb7673bff331c95e
--
Peter Geoghegan
d of execution time smarts about the statistics, and
their inherent unreliability. Somehow query execution itself should
become less gullible, at least in cases where we really can have high
confidence in the statistics being wrong at this exact time, for this
exact key space.
[1]
https://postgr.es/m/CAH2-Wz=9r83wcwzcpuh4fvpedm4znzbzmvp3rt21+xhqwmu...@mail.gmail.com
--
Peter Geoghegan
ether or not we need to involve
contrib/amcheck itself doesn't seem important to me right now. Offhand
I think that we wouldn't, because as you point out pg_catcheck is a
frontend program that checks multiple databases.
--
Peter Geoghegan
patch, and
submitted. That would give us very broad coverage.
--
Peter Geoghegan
ckpatching anything
here.
The problem report from Nikolay was probably an unusually bad case,
where the pending list cleanup/insertion was particularly expensive.
This *is* really awful behavior, but that alone isn't a good enough
reason to backpatch.
--
Peter Geoghegan
On Thu, Oct 14, 2021 at 2:31 PM Mark Dilger
wrote:
> I was just waiting a couple minutes to see if Andrew wanted to jump in.
> Having heard nothing, I guess you can revert it.
Okay. Pushed a commit removing the test case just now.
Thanks
--
Peter Geoghegan
an investigation on affected BF
animals, or something else?
> Or do you have a different way forward for that?
I don't know enough about this stuff to be able to comment.
--
Peter Geoghegan
I agree. I can go remove the whole file now, and will.
Mark: Any objections?
--
Peter Geoghegan
n't be 24 bytes long.
That's the same as an nbtree page, which confirms my suspicion. The 20
bytes consists of a 16 byte tuple, plus a 4 byte line pointer. The
tuple-level alignment overhead gets you from 12 bytes to 16 bytes with
a single int4 column. So the padding is there for the taking.
--
Peter Geoghegan
On Wed, Oct 13, 2021 at 9:15 PM Tom Lane wrote:
> Looks like a transient/phase of the moon issue to me.
Yeah, I noticed that drongo is prone to them, though only after I hit send.
Thanks
--
Peter Geoghegan
On Wed, Oct 13, 2021 at 2:09 PM Peter Geoghegan wrote:
> I just pushed v4, with the additional minor pg_amcheck documentation
> updates we talked about. No other changes.
Any idea what the problems on drongo are?
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2021-10-1
On Sat, Oct 9, 2021 at 5:51 PM Peter Geoghegan wrote:
> While I'm no closer to a backpatchable fix than I was on Thursday, I
> do have some more ideas about what to do on HEAD. I now lean towards
> completely ripping analyze_only calls out, there -- the whole idea of
> calling amv
gly about it either.
I just pushed v4, with the additional minor pg_amcheck documentation
updates we talked about. No other changes.
Thanks again
--
Peter Geoghegan
On Wed, Oct 13, 2021 at 12:15 PM Peter Geoghegan wrote:
> Are you sure? I know that nbtree index tuples for a single-column int8
> index are exactly the same size as those from a single column int4
> index, due to alignment overhead at the tuple level. So my guess is
> that hash
So my guess is
that hash index tuples (which use the same basic IndexTuple
representation) work in the same way.
--
Peter Geoghegan
sume that it is bug compatibility. Is that intrinsically a bad thing?
--
Peter Geoghegan
lls us that there are bound
to be some number of users relying on it. I don't think that it's
worth inconveniencing those people without getting a clear benefit in
return.
Being lenient here just doesn't have much downside in practice, as
evidenced by the total lack of complaints about that lenience.
--
t also seems unlikely to make any appreciable difference
to the overall level of coverage in practice.
Would it be simpler to do it all together, in
compile_relation_list_one_db()? Were you concerned about things
changing when parallel workers are run? Or something else?
Many thanks
--
Peter Geoghegan
secondary point, at best. We don't need to use a
warning box because of that.
Overall, your approach looks good to me. Will Robert take care of
committing this, or should I?
--
Peter Geoghegan
On Mon, Oct 11, 2021 at 11:40 AM Peter Geoghegan wrote:
> A NOTICE message is supposed to be surfaced to clients (but not stored
> in the server log), pretty much by definition.
>
> It's not unreasonable to argue that I was mistaken to ever think that
> about this particular mess
ditive thing.
Totally changing the general locking requirements seems like a POLA
violation. Besides, amcheck proper is now very much the low level tool
that most users won't ever bother with.
--
Peter Geoghegan
easonably complain about this on a hot standby with
> millions of unlogged relations. Actual ERROR messages might get lost in all
> the noise.
That's a good point.
--
Peter Geoghegan
> style.
I definitely think that it warrants a warning box. This is a huge
practical difference.
Note that I'm talking about a standard thing, which there are
certainly a dozen or more examples of in the docs already. Just grep
for " " tags to see the existing warning boxes.
--
Peter Geoghegan
On Mon, Oct 11, 2021 at 11:12 AM Peter Geoghegan wrote:
> Sure, the user might not be happy with --parent-check throwing an
> error on a replica. But in practice most users won't want to do that
> anyway. Even on a primary it's usually not possible as a practical
> matter, because
On Mon, Oct 11, 2021 at 10:46 AM Mark Dilger
wrote:
> > On Oct 11, 2021, at 10:10 AM, Peter Geoghegan wrote:
> > This mostly looks good to me. Just one thing occurs to me: I suspect
> > that we don't need to call pg_is_in_recovery() from SQL at all. What's
> >
when that's the logical outcome (e.g., when --parent-check is
used on a replica). To me this seems rather different than not
checking temporary tables, because that's something that inherently
won't work. (Also, I consider the index-is-being-built stuff to be
very similar to the temp table stuff -- same basic situation.)
--
Peter Geoghegan
But experience suggests that they
don't.
--
Peter Geoghegan
On Thu, Oct 7, 2021 at 10:34 PM Peter Geoghegan wrote:
> This issue was brought to my attention by Nikolay Samokhvalov. He
> reached out privately about it. He mentioned one problematic case
> involving an ANALYZE lasting 45 minutes, or longer (per
> log_autovacuum_min_dur
c-postgresql.com/en/reasons-why-vacuum-wont-remove-dead-rows/
--
Peter Geoghegan
could be used as a fix for
this issue, at least on HEAD.
[1] https://postgr.es/m/20210405063117.GA2478@ahch-to
--
Peter Geoghegan
r had
-- having only one pg_class entry was actually the true underlying
problem, all along.
Sometimes we have to make a difficult choice between "weird rules but
nice behavior" (as with REINDEX CONCURRENTLY), and "nice rules but
weird behavior" (as with plain REINDEX).
--
Peter Geoghegan
question, and I don't think you need to weigh in
> unless you want to. I'll most likely go with whatever is the simplest to
> code and/or most similar to what is currently in the tree, because I don't
> see any knock-down arguments one way or the other.
I agree with you that this is a UI thing, since in any case the temp
table is pretty much "not visible to pg_amcheck". I have no particular
feelings about it.
Thanks
--
Peter Geoghegan
downgrade options as necessary, but now it sounds like
> that's not so. So what should it do
Downgrade is how you refer to it. I just think of it as making sure
that pg_amcheck only asks amcheck to verify relations that are
basically capable of being verified (e.g., not a temp table).
--
Peter Geoghegan
things, in one way or
another.
> No, that's existing code from btree_index_mainfork_expected. I thought
> you were saying that verify_heapam.c should adopt the same approach,
> and I was agreeing, not because I think it's necessarily the perfect
> approach, but for consistency.
Sorry, I somehow read that code as having an ERROR, not a NOTICE.
(Even though I wrote the code myself.)
--
Peter Geoghegan
gt; can't check an unlogged relation on a standby than get nothing. Sure, what I
> did doesn't make sense, but why should the application paper over that
> mistake?
I think that it shouldn't get an error at all -- this should be
treated like an empty relation, per the verify_nbtree.c pr
omoted, we'll
then also have no on-disk storage for the same unlogged relation, but
now suddenly it's okay, just because of that. I find it far more
logical to just assume that there is no relfilenode storage to check
when in hot standby.
This isn't the same as the --parent-check thing at all, because that's
about an implementation restriction of Hot Standby. Whereas this is
about the physical index structure itself.
--
Peter Geoghegan
On Wed, Oct 6, 2021 at 11:55 AM Peter Geoghegan wrote:
> I am pretty sure that I agree with you about all these details. We
> need to tease them apart some more.
I think that what I've said boils down to this:
* pg_amcheck shouldn't attempt to verify temp relations, on the
g
the indexes
(actually just show those NOTICE messages), or skip them entirely. I
lean towards the former option, on the grounds that I don't think it
should be special-cased. But I don't feel very strongly about it.
--
Peter Geoghegan
. It is uncomfortable to me that `pg_amcheck
> --parent-check` will now silently not perform the parent check that was
> explicitly requested.
But the whole definition of "check that was explicitly requested"
relies on your existing understanding of what pg_amcheck is supposed
to do. That's not actually essential. I don't see it that way, for
example.
--
Peter Geoghegan
inition not corrupt
>
> I think #1 and #3 are unsurprising enough that they need no documentation
> update. #2 is slightly surprising (at least to me) so I updated the docs for
> it.
To me #2 sounds like a tautology. It could almost be phrased as
"pg_amcheck does not check the things that it cannot check".
--
Peter Geoghegan
e happens to be even one such index.
And yet the return value from pg_amcheck is still 0 (barring problems
elsewhere). I think that it'll always be possible to make *some*
argument like that, even in a world where pg_amcheck + amcheck are
very feature complete. As I said, it seems fundamental.
On Tue, Oct 5, 2021 at 10:21 PM Jaime Casanova
wrote:
> At this time I'm looking to close the CF.
Thanks, Jaime.
--
Peter Geoghegan
d way, that
creates ambiguity. Ordinary DDL certainly doesn't count as unusual
here.
--
Peter Geoghegan
ses to enforce work_mem limits sometimes allows the
implementation to use slightly more memory than theoretically
allowable. That's how we get negative values.
--
Peter Geoghegan
r working on the issue so promptly.
This was a question of fundamental definitions. Those are often very tricky.
--
Peter Geoghegan
On Tue, Oct 5, 2021 at 9:41 AM Robert Haas wrote:
> On Mon, Oct 4, 2021 at 7:10 PM Peter Geoghegan wrote:
> > All of the underlying errors are cases that were clearly intended to
> > catch user error -- every single one. But apparently pg_amcheck is
> > incapable of error
On Mon, Oct 4, 2021 at 12:09 PM Jaime Casanova
wrote:
> - Extending amcheck to check toast size and compression
> Last patch from may-2021, also it seems there is no activity since
> Jul-2021. Peter Geoghegan, are you planning to look at this one?
I didn't plan on it, no.
uld be happy to work with you on this. It's clearly an important project.
Having you involved with the core planner aspects (as well as general
design questions) significantly derisks everything. That's *very*
valuable to me.
--
Peter Geoghegan
st happen.
> So I am changing pg_amcheck to filter out indexes when pg_is_in_recovery() is
> true and relpersistence='u'. Does that sound right to you?
Yes, that all sounds right to me.
Thanks
--
Peter Geoghegan
require the SQL caller to do anything like what you are
> requiring here in order to prevent deadlocks, and also because the docs for
> the functions don't say that a deadlock is possible, merely that the function
> may return with an error.
I will need to study the deadlock issue separately.
--
Peter Geoghegan
ly what
they'd expect -- verification of the original index, without any
hindrance from the new/failed index.
(Thinks for a moment...)
Actually, I think that we'd only verify the original index, even
before the error with CONCURRENTLY (though I've not checked that point
myself).
--
Peter Geoghegan
ated as
axiomatic truths about how pg_amcheck must work. Should we now "fix"
pg_dump so that it matches pg_amcheck?
All of the underlying errors are cases that were clearly intended to
catch user error -- every single one. But apparently pg_amcheck is
incapable of error, by definition. Like HAL 9000.
--
Peter Geoghegan
e really don't want to have to
throw an "invalid object type" style error just because verification
runs during recovery. Plus it just seems logical to assume that
unlogged indexes/tables don't have storage when we're in hot standby
mode, and so must simply have nothing for us to verify.
--
Peter Geoghegan
cess 9555.
> HINT: See server log for query details.
> ERROR: cannot check index "t_idx"
> DETAIL: Index is not valid.
I think that the deadlock is just another symptom of the same problem.
--
Peter Geoghegan
without reaching commit, we should either kill it or
> form a specific plan for how to advance it.
Also fair.
The pandemic has made the kind of coordination I refer to harder in
practice. It's the kind of thing that face to face communication
really helps with.
--
Peter Geoghegan
ee stuff here,
which is clearly my thing. But I know that I personally am much less
likely to work on a patch that requires significant changes to the
planner. Maybe this is a coordination problem.
--
Peter Geoghegan
On Fri, Oct 1, 2021 at 2:35 PM Bossart, Nathan wrote:
> On 9/30/21, 3:44 PM, "Peter Geoghegan" wrote:
> > I will commit this patch in a few days, barring objections.
>
> +1
Okay, pushed.
Thanks
--
Peter Geoghegan
wnside, as long as we just use the resources that they make
available for development work. Clearly these services could never
replace the buildfarm.
--
Peter Geoghegan
On Wed, Sep 29, 2021 at 3:32 PM Peter Geoghegan wrote:
> I decided to run a simple experiment, to give us some idea of what
> benefits my proposal gives users: I ran "make installcheck" on a newly
> initdb'd database (master branch), and then with the attached patch
> (which
On Wed, Sep 29, 2021 at 11:27 AM Peter Geoghegan wrote:
> I would like to enable deduplication within system catalog indexes for
> Postgres 15.
I decided to run a simple experiment, to give us some idea of what
benefits my proposal gives users: I ran "make installcheck" on
o have a fair number of duplicates.
--
Peter Geoghegan
On Fri, Sep 24, 2021 at 7:44 PM Peter Geoghegan wrote:
> The scheduling of autovacuum is itself a big problem for the two big
> BenchmarkSQL tables I'm always going on about -- though it did get a
> lot better with the introduction of the
> autovacuum_vacuum_insert_scale_factor stuff
d-f106-8abde596c0e4%40commandprompt.com
[2]
https://postgr.es/m/CAH2-Wz=sJm3tm+FpXbyBhEhX5tbz1trQrhG6eOhYk4-+5uL=w...@mail.gmail.com
--
Peter Geoghegan
On Thu, Sep 23, 2021 at 10:42 PM Masahiko Sawada wrote:
> On Thu, Sep 16, 2021 at 7:09 AM Peter Geoghegan wrote:
> > Enabling index-only scans is a good enough reason to pursue this
> > project, even on its own.
>
> +1
I was hoping that you might be able to work on oppor
discretion to keep the website
version of the release notes a bit more consistent then what you'd get
from the RC1 tarball.
I'm not going to make a fuss about it, but it would have been nice if
we'd kept with the usual schedule for the major features list.
--
Peter Geoghegan
sed in less than 24 hours. ISTM that we're
almost out of time.
Is some kind of simple compromise possible?
--
Peter Geoghegan
On Sun, Sep 19, 2021 at 7:47 PM Peter Geoghegan wrote:
> Attached patch fixes the bug by slightly narrowing the conditions
> under which we'll consider if we should apply deduplication's single
> value strategy.
Pushed this fix a moment ago.
--
Peter Geoghegan
ck
whether or not we just had a "failed" call to _bt_bottomupdel_pass()
-- this is a logical extension of what we do already. Barring
objections, I will apply this patch (and backpatch to Postgres 14) in
a few days.
--
Peter Geoghegan
v1-0001-Fix-single-value-strategy-index-deletion
ex deletion pass is attempted for the same
page, we'll probably find something to delete.
Just accepting version-driven page splits was always a permanent
solution to a temporary problem.
> Thank you so much for your work on this!
Thanks Marko!
--
Peter Geoghegan
now think
that it's fairly common. I also like your idea here because it enables
a more qualitative approach, based on recent information for recently
modified blocks -- not whole-table statistics. Averages are
notoriously misleading.
[1] https://github.com/pgsql-io/benchmarksql/pull/16
--
Peter
entation.html#BTREE-DELETION
--
Peter Geoghegan
On Mon, Sep 13, 2021 at 5:36 PM Peter Geoghegan wrote:
> If somebody wants to make TIDs (or some generalized TID-like thing
> that tableam knows about) into logical identifiers, then they must
> also answer the question: identifiers of what?
>
> TIDs from Postgres heapam iden
ariable-length TID in the future? Or make it more difficult?
>
> How much work would it take?
If it was just a matter of changing the data structure then I think it
would be far easier.
--
Peter Geoghegan
e said, I
think it's true that the system does the right thing in most
situations -- even with BenchmarkSQL/TPC-C! I just find it useful to
focus on the exceptions, the cases where the system clearly does the
wrong thing.
--
Peter Geoghegan
aturally start to fail, e.g. prune-before-evict
> won't work once the operation becomes large enough that pages have to
> be evicted while the transaction is still running.
Perhaps. As you know I'm generally in favor of letting things fail
naturally, and then falling back on an alternative strategy.
--
Peter Geoghegan
end to do some kind of batching, but only at the level of small
groups of related transactions. Writing a page that was quickly opened
for new inserts, filled with newly inserted heap tuples, then closed,
and finally cleaned up doesn't seem like it needs to take any direct
responsibility for writeback.
--
Peter Geoghegan
t; transaction become so large that it
just doesn't make sense to do either? What's the crossover point at
which background processing and foreground processing like this should
be assumed to be not worth it? That I won't speculate about just yet.
I suspect that at some point it really does make sense to leave it all
up to a true table-level batch operation, like a conventional VACUUM.
--
Peter Geoghegan
g work or conflicting requirements. The
resource management aspects of my own prototype are the least worked
out and shakiest parts overall, so this seems likely. You've probably
already solved numerous problems for me.
--
Peter Geoghegan
e
backend (for the same table).
The work of setting hint bits and pruning-away aborted heap tuples has
to be treated as a logical part of the cost of inserting heap tuples
-- backends pay this cost directly. At least with workloads where
transactions naturally only insert a handful of rows each in almost
all cases -- very much the common case.
--
Peter Geoghegan
On Mon, Aug 16, 2021 at 5:15 PM Peter Geoghegan wrote:
> This is actually quite simple -- the chaos that follows is where it
> gets truly complicated (and not necessarily worth getting into just
> yet). The update of a given order and its order lines entries takes
> place hours l
sers -- it can be
recast as information about index tuples specifically (there may not
actually be any matching index tuples, especially in Postgres 14, but
that isn't worth getting in to in user docs IMV).
--
Peter Geoghegan
the series, relative to the v2 from
> > earlier.)
>
> Looks alright to me.
Pushed both patches -- thanks.
--
Peter Geoghegan
he
second patch concerns this separate track_io_timing issue. It's pretty
straightforward.
(No change to the first patch in the series, relative to the v2 from earlier.)
--
Peter Geoghegan
v3-0002-Show-zero-values-in-track_io_timing-log-output.patch
Description: Binary data
v3-0001-Reorder-log_aut
n vacuumlazy.c and in analyze.c), without making any
other changes (i.e., no changes to EXPLAIN output are needed).
You seem to be almost sold on that plan anyway. But this text format
EXPLAIN rule seems like it decides the question for us.
--
Peter Geoghegan
On Thu, Aug 26, 2021 at 10:28 PM Peter Geoghegan wrote:
> I'll commit this in a day or two, backpatching to 14. Barring any objections.
Actually, we also need to make the corresponding lines for ANALYZE
follow the same convention -- those really should be consistent. As in
the attached revis
th things
(both "read:" and "write:"), without regard for whether or not they're
non-zero. (We'd do the same thing with ANALYZE's equivalent code too,
if we actually did this -- same issue there.)
--
Peter Geoghegan
On Wed, Aug 25, 2021 at 5:02 PM Peter Geoghegan wrote:
> > I like it better than the current layout, so +1.
>
> This seems like a release housekeeping task to me. I'll come up with
> a patch targeting 14 and master in a few days.
Here is a patch that outputs log_autovacuum's line
Patch committed.
Thanks!
--
Peter Geoghegan
On Thu, Aug 26, 2021 at 4:24 PM Mark Dilger
wrote:
> > On Aug 26, 2021, at 2:38 PM, Peter Geoghegan wrote:
> > It looks like we neglect to call CHECK_FOR_INTERRUPTS() anywhere
> > inside verify_heapam.c. Is there any reason for this?
>
> Not any good one that I can se
e that I can benefit from whatever work Andres has already done
on this, particularly when it comes to managing per-relation metadata
in shared memory.
--
Peter Geoghegan
1201 - 1300 of 3113 matches
Mail list logo
