eed to worry about it, rather than
that they're wrong to ask about it. Though we should probably actually
just throw an error.
--
Peter Geoghegan
s
what you said, though.
> +1 for ii_IndexAttrNumbers.
+1
--
Peter Geoghegan
ets truncated away.
--
Peter Geoghegan
it looks like the tests added to update.sql by commit
2f178441 ("Allow UPDATE to move rows between partitions") lead to this
failure, since I always hit a problem when update.sql is reached. I
haven't gone to the trouble of digging any deeper than that just yet.
--
Peter Geoghegan
On Thu, Apr 12, 2018 at 11:47 AM, Peter Geoghegan <p...@bowt.ie> wrote:
> In short, it looks like the tests added to update.sql by commit
> 2f178441 ("Allow UPDATE to move rows between partitions") lead to this
> failure, since I always hit a problem when update.sql is
ailover scripts of we
> backpatch it, isn't it?
+1 to committing to master as a documented behavioral change.
--
Peter Geoghegan
ing about (perhaps
just renaming the macro).
I do not have the time to write a patch right away, but I should be
able to post one in a few days. I want to avoid sending several small
patches.
--
Peter Geoghegan
oblems. I have personally scoffed at stories
involving Postgres databases corruption that gets attributed to
running out of disk space. Looks like I was dead wrong.
[1] https://danluu.com/file-consistency/ -- "Filesystem correctness"
--
Peter Geoghegan
bit dubious... should probably be returning NULL for most
> properties except 'returnable'.
>
> I can look at fixing these for you if you like?
I'm happy to accept your help with it, for one.
--
Peter Geoghegan
ed Pavan to at least add a note to the nbtree README that
explains the high level theory behind the optimization, as part of
post-commit clean-up. I'll ask him to say something about how it might
affect extent-based storage, too.
--
Peter Geoghegan
t once
per page. Is it worth noting the exception?
--
Peter Geoghegan
wouldn't have made a big difference in the
end. There is only so much time available.
--
Peter Geoghegan
I right?
Yes.
I'm going to look at this again today, and will post something within
12 hours. Please hold off on committing until then.
--
Peter Geoghegan
ms.
>
> What do you think about applying following patch on the top of v14?
It's clearly necessary. Looks fine to me.
--
Peter Geoghegan
ot within
scope for v11. In some cases, these were addressed with only modest
effort. It's pretty obvious to me that some aspects of MERGE's
architecture are accidents. Not necessarily happy accidents.
--
Peter Geoghegan
On Tue, Apr 10, 2018 at 5:45 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> I did find another problem, though. Looks like the idea to explicitly
> represent the number of attributes directly has paid off already:
>
> pg@~[3711]=# create table covering_bug (f1 int, f2 int, f3 text
On Tue, Apr 10, 2018 at 1:37 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> _bt_mark_page_halfdead() looked like it had a problem, but it now
> looks like I was wrong.
I did find another problem, though. Looks like the idea to explicitly
represent the number of attributes directly
the instance just needs to be offline, which can be anything as long as
> the postmaster is stopped. That's how I understand the current
> wording.
I see. The problem is clearly the documentation, then.
--
Peter Geoghegan
On Sat, Apr 14, 2018 at 8:02 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> So we've got *some*, but it sure looks like they were all added by the
> patch to fix covering indexes for partitions. I'd want to see some for
> plain-table cases as well.
Will do.
--
Peter Geoghegan
then didn't replace the equivalent subexpression
> of MaxIndexTuplesPerPage with MinIndexTupleSize?".
I had the same reaction.
--
Peter Geoghegan
a computationally intensive
process; CPU costs are bound to dominate. A strategy like this would
also make it easier to run amcheck on indexes left behind by the
regression tests.
How can we do better?
--
Peter Geoghegan
tch added tests to. At no point do they leave behind any
INCLUDE indexes. I'll do something about that as part of the INCLUDE
patch that I'm working on at the moment.
--
Peter Geoghegan
ad and contains some comment
> improvement in index_truncate_tuple() - you save some amount of memory with
> TupleDescCopy() call but didn't explain why pfree is enough to free all
> allocated memory.
Makes sense.
--
Peter Geoghegan
your database.
I'm fine if Teodor wants to commit that change separately, of course.
--
Peter Geoghegan
ttached patch contains all
> changes suggested in my previous email.
Looks new BTreeTupSetNAtts () assertion good to me.
I suggest committing this patch as-is.
Thank you
--
Peter Geoghegan
350ms is short enough to place it in both core and
> amcheck test. I think, basic functionality should be covered by core tests
> as we test insert/create.
I think you're right. There is really no excuse for not having
thorough code coverage for a module like nbtree.
--
Peter Geoghegan
infinity
items to the same comment block. While it's true that _bt_insertonpg()
cannot truncate downlinks to make new minus infinity items, I see that
as a narrower issue.
--
Peter Geoghegan
From dced00be29775965a45c7889ca99e19b96d9e4d0 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.i
the nbtree README for full details."
--
Peter Geoghegan
ound the
absence of a needed downlink. There is an absent downlink, but that's
beside the point, and in any case missing a downlink is not
*inherently* wrong (as I said, it's not inherently wrong because of
the issue of interrupted multi-level page deletes).
--
Peter Geoghegan
on't drop all tables in the regression tests, so that we have some
pg_dump coverage for INCLUDE indexes, per a request from Tom.
[1] https://queue.acm.org/detail.cfm?id=2220317
--
Peter Geoghegan
From db777cad48f185afb70c589beae15fa166c0ddf1 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.
On Wed, Apr 18, 2018 at 10:29 PM, Teodor Sigaev <teo...@sigaev.ru> wrote:
> Will see...
I'll take a look tomorrow.
--
Peter Geoghegan
gorithm in any way.
> Trivial and naive fix is attached, but for me it looks a bit annoing that we
> store pointer (leafhikey) somewhere inside unlocked page.
Well, it has worked that way for a long time. No reason to change it now.
I also think that we could have better conventional regression test
coverage here.
--
Peter Geoghegan
ng palloc0(). Done that way.
--
Peter Geoghegan
From 36bd9387542fa594c03877b4f0c90211dc6a4c21 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.ie>
Date: Thu, 19 Apr 2018 17:45:26 -0700
Subject: [PATCH v2] Add missing and dangling downlink checks to amcheck.
When bt_index_parent_che
On Sat, Apr 21, 2018 at 6:02 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> I refined the amcheck enhancement quite a bit today. It will not just
> check that a downlink is not missing; It will also confirm that it
> wasn't a legitimately interrupted multi-level deletion, by descendin
that's got to change.
Andres mentioned that he has prototyped an approach to buffer
management that uses a Radix tree, which is generally assumed to be
the right long-term fix.
--
Peter Geoghegan
think we can have any confidence that
> nothing will break unless we do a detailed audit of all that code.
+1. I think that it is plainly a bad idea to do something like that at
this point in the cycle.
--
Peter Geoghegan
a good idea? This could get pretty expensive if it
was overused, even by the standards of what we expect from
assertion-enabled builds, but we could make it optional if the
overhead got out of hand.
--
Peter Geoghegan
On Wed, Apr 18, 2018 at 1:45 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> I suggest committing this patch as-is.
Actually, I see one tiny issue with extra '*' characters here:
> +* The number of attributes won't be explicitly represented if the
> +* negative
l bugs when it was first added.
>
> Yep.
I wrote a simple prototype of this. No assertion failed during a "make
installcheck". Assertions were added to heap_tuple_fetch_attr(), and a
couple of other places.
--
Peter Geoghegan
top of
heap_tuple_fetch_attr(). AssertNotInCriticalSection() certainly found
several bugs when it was first added.
--
Peter Geoghegan
ee.
In summary, I think that we should find a way to never use
ItemPointerGetBlockNumberNoCheck() in nbtree, while still using it in
amcheck.
--
Peter Geoghegan
e already). Maybe we can add it to the amcheck regression tests
instead, since those are run less often. This also allows us to add a
test case specifically as part of the amcheck enhancement, which makes
a bit more sense.
--
Peter Geoghegan
to make
work with pg_regress. :-)
It looks like you pushed v1, which didn't have the tests and other
changes you asked for. Attached patch adds those back.
Thanks
--
Peter Geoghegan
From fc2d158d3d1c52e94a6d3f66c65027bf09b64f7f Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.ie>
Date: Thu,
view. I hope that this can be committed shortly.
--
Peter Geoghegan
From ede1ba731dc818172a94adbb6331323c1f2b1170 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.ie>
Date: Thu, 24 Aug 2017 20:58:21 -0700
Subject: [PATCH v7 1/2] Add Bloom filter data structure implementation.
A Bloom
s.
Also, we could use suffix truncation to save at least one byte in
almost all cases, even with the thinnest possible
single-integer-attribute IndexTuples. What you describe just isn't
going to happen.
--
Peter Geoghegan
t you not mark the patch as ready
for committer, actually. I was just expressing that your input was
valuable. Sorry for being unclear.
--
Peter Geoghegan
ojections. That's what we see for the RETURNING +
ON CONFLICT projections within nodeModifyTable.c, which the new
projections are very similar to, and clearly modeled on.
--
Peter Geoghegan
o rely on catalog metadata about this being an INCLUDE index.
--
Peter Geoghegan
e difference is that there are no hard-coded
assumptions about the relationship between those two RTEs.
--
Peter Geoghegan
ns about dropping the function and
> causing issues due to dependencies.
WFM. I have all the information I need to produce the next revision now.
--
Peter Geoghegan
x_check_heap() +
bt_index_parent_check_heap()? Or, are you talking about function
overloading?
--
Peter Geoghegan
On Sat, Mar 31, 2018 at 2:59 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> WFM. I have all the information I need to produce the next revision now.
I might as well post this one first. I'll have 0002 for you in a short while.
--
Peter Geoghegan
v8-0001-Add-Bloom-filter-data-
On Sat, Mar 31, 2018 at 3:15 PM, Peter Geoghegan <p...@bowt.ie> wrote:
>> WFM. I have all the information I need to produce the next revision now.
>
> I might as well post this one first. I'll have 0002 for you in a short while.
Attached is 0002 -- the amcheck enhancement it
ad of having an
accurate high key size for every candidate split point would be
significant.
--
Peter Geoghegan
On Thu, Mar 29, 2018 at 7:42 PM, Peter Geoghegan <p...@bowt.ie> wrote:
>> We should be able to get this into v11...
>
> That's a relief. Thanks.
I have a new revision lined up. I won't send anything to the list
until you clear up what you meant in those few cases where it seeme
On Sat, Mar 31, 2018 at 8:09 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> I was thinking of using rint(), which is what you get if you call
> round(float8) from SQL.
Attached patch does it that way. Note that there are float/int cast
regression tests that ensure that rint() behaves
On Sat, Mar 31, 2018 at 8:32 PM, Andres Freund <and...@anarazel.de> wrote:
> LGTM, pushed. Closing CF entry. Yay! Only 110 to go.
Thanks Andres!
--
Peter Geoghegan
On Sat, Mar 31, 2018 at 3:15 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> On Sat, Mar 31, 2018 at 2:59 PM, Peter Geoghegan <p...@bowt.ie> wrote:
>> WFM. I have all the information I need to produce the next revision now.
>
> I might as well post this one first. I'll h
round(float8) from SQL.
--
Peter Geoghegan
its table. Especially one that can run
without a connection to the database, for things like backup tools,
where performance is really important. There is certainly room for
that. For this particular enhancement, the similarity to CREATE INDEX
seems like an asset.
--
Peter Geoghegan
so looked at the 0001 patch and it looks fine to me.
I'm grateful that you didn't feel any need to encourage me to use
whatever the novel/variant filter du jour is! :-)
--
Peter Geoghegan
e fact, and
as a general smoke-test, where it works well. I would perhaps
recommend running it once a week, although that's a fairly arbitrary
choice. The docs in v10 don't take a position on this, so while I tend
to agree we could do better, it is a preexisting issue.
[1] https://www.postgresql.org/docs/10/static/amcheck.html#id-1.11.7.11.7
--
Peter Geoghegan
comprehensible to
> users.
+1. You make a good point about Java. Furthermore, when people talk
about just in time compilation in database systems, which is far from
a novel thing, they generally use the word JIT.
Why confuse things?
--
Peter Geoghegan
ear why,
and I think it needs to be explained.
--
Peter Geoghegan
necessitates the use of multiple RTEs. I don't think it would make
sense to use a second RTE only when needed.
--
Peter Geoghegan
, I don't think it's worth.
Clearly I jumped the gun on this one. I agree that this is fine.
--
Peter Geoghegan
depth=3, with 3 being determined
> purely experimentally though.
A quick look at the DWARF4 standard [1] suggests that this refers to
lexical depth. So, I agree that that doesn't seem like a great idea.
[1] http://dwarfstd.org/doc/DWARF4.pdf
--
Peter Geoghegan
orry for going off in a tangent, but I think it's somewhat necessary
to have a strategy here. Of course, we don't have to get everything
right now, but we should be looking in this direction whenever we talk
about on-disk nbtree changes.
--
Peter Geoghegan
wiki/Key_normalization#How_big_can_normalized_keys_get.2C_and_is_it_worth_it.3F
--
Peter Geoghegan
of the user docs -- the section that
3785f7ee added.
[1]
https://postgr.es/m/cah2-wzm9y59h2m6izjm4fpdup5r4bsrvzgbn2gtrco1j4nz...@mail.gmail.com
[2]
https://github.com/postgrespro/postgrespro/blob/PGPRO9_5/src/backend/access/common/indextuple.c#L451
--
Peter Geoghegan
I guess we can add that later, but it's a bit sad that this won't work
> against views with INSTEAD OF triggers.
It also makes it hard to test deparsing support, which actually made
it in in the end. That must be untested.
--
Peter Geoghegan
n error occurred somewhere in the file is all that is necessary;
applications that require better granularity already use O_DIRECT."
--
Peter Geoghegan
rst difficulty isn't high
> too.
I think it's possible.
I didn't have time to look at this properly today, but I will try to
do so tomorrow.
Thanks
--
Peter Geoghegan
you for your patience and
perseverance, despite what I see as mistakes in how this project was
manged. I really want for it to be possible for there to be more
patches in the nbtree code, because they're really needed. That was a
big part of my motivation for writing amcheck, in fact. It's tedious
to link this patch to a bigger picture about what we need to do with
nbtree in the next 5 years, but I think that that's what it will take
to get this patch in. That's my opinion.
--
Peter Geoghegan
hat isn't ideal, and
particularly hinders automated testing.
--
Peter Geoghegan
of page splits, overflows, consolidations, and
removals must be carefully reviewed to ensure that predicate locks
aren't "lost" during those operations, or kept with pages which could
get re-used for different parts of the index.
Do we need to worry about that here? I guess not, because this is just
like having many duplicates. But a note just above the _bt_doinsert()
call to CheckForSerializableConflictIn() might be a good idea.
That's all I have for today.
--
Peter Geoghegan
both of them...
I think that the thing that's more likely to kill this patch is the
fact that after the first year, it only ever got discussed in the
final CF. That's not something that happened because of my choices. I
made several offers of my time. I did not create this urgency.
[1] https://www.
On Fri, Mar 30, 2018 at 10:39 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> It's safe, although I admit that that's a bit hard to see.
> Specifically, look at this code in _bt_insert_parent():
>
> /*
> * Find the parent buffer and get the parent page.
>
On Fri, Mar 30, 2018 at 6:55 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> On Fri, Mar 30, 2018 at 6:20 PM, Andres Freund <and...@anarazel.de> wrote:
>>> What would the upcasting you have in mind look like?
>>
>> Just use UINT64CONST()? Let's try not to introduce
t I don't see that there is much more
that we can do while starting with a B-Tree index as our target.
Please propose an alternative user interface for the new check. If you
prefer, I can invent new bt_index_check_heap() +
bt_index_parent_check_heap() variants. That would be okay with me.
--
Peter Geoghegan
ng heap tuples look dead when they shouldn't
be. But it's an opportunity to add less stuff to the Bloom filter,
which might make a small difference. Also, it might have some minor
educational value, for hackers that want to learn more about nbtree,
which remains a secondary goal.
>> +/*
>> + * Per-tuple callback from IndexBuildHeapScan, used to determine if index
>> has
>> + * all the entries that definitely should have been observed in leaf pages
>> of
>> + * the target index (that is, all IndexTuples that were fingerprinted by our
>> + * Bloom filter). All heapallindexed checks occur here.
>
> The last sentence isn't entirely fully true ;), given that we check for
> the bloom insertion above. s/checks/verification/?
Not sure what you mean. Is your point that we could have an error from
within IndexBuildHeapScan() and friends, as opposed to from this
callback, such as the error that we saw during the "freeze the dead"
business [3]?
The bloom_add_element() calls cannot raise errors; they're just there
because they're needed to make the check in the callback
bt_tuple_present_callback() have a summarizing structure to work off
of.
> We should be able to get this into v11...
That's a relief. Thanks.
[1]
https://postgr.es/m/cah2-wznm5zojs0_djowrcm9us19gzbkm0atkt5hhprvjhfv...@mail.gmail.com
[2] https://hur.st/bloomfilter/
[3]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=8ecdc2ffe3da3a84d01e51c784ec3510157c893b
--
Peter Geoghegan
.
This cross-check workaround is ugly, but apparently there is a
precedent in copy.c. I didn't know that detail until Robert pointed it
out. That makes me feel a lot better about this general question of
how the target relation is represented, having two RTEs, etc.
--
Peter Geoghegan
quires. It's not a special case; it's exactly as bad as any
statement that takes the same amount of time to execute.
> Is
> there anything we can do to lessen that burden like telling other backends
> to
> ignore our xmin while computing OldestXmin (like vacuum does)?
I don't th
that; wouldn't it
> be simpler to extract those members from the node?
> Or is there another reason to pass the index list?
It works that way pretty much by accident, as far as I can tell.
Removing the two extra arguments sounds like a good idea.
--
Peter Geoghegan
t/uploads/2009/09/DualPivotQuicksort.pdf
[3]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-September/002630.html
--
Peter Geoghegan
first normal form, and the
fact that a high start up cost for the sort is already very likely. It
is a technique specifically tailored for a database system, and modern
hardware characteristics.
--
Peter Geoghegan
n that specifically
targets a heap relation (not an index), and performs verification
fairly quickly, so my heapallindexed patch isn't enough. That wouldn't
share much with the existing amcheck verification functions. I hope
that someone else can pick that up soon.
--
Peter Geoghegan
it describes doesn't
seem to have much to do with MVCC rules per say. The only thing that
seems to warrant discussion in mvcc.sgml is how MERGE really *isn't* a
special case. ISTM that you only really need to mention how the
decision to use one particular WHEN action can change repeatedly -
every time you walk the UPDATE chain, you start that part from the
beginning.
The "you might get a duplicate violation" bit can definitely live in
merge.sgml, right at the point that ON CONFLICT is mentioned (the Tip
box). I don't think that you need too much on this.
--
Peter Geoghegan
ICT is a clause.
>
> I think it would be better if you wrote a separate additional doc
> patch to explain all of this, perhaps in Performance Tips section or
> otherwise.
I don't think that it has much to do with performance.
--
Peter Geoghegan
ut the overall
design that likely creates problems elsewhere. It's easy to say that
it isn't worth holding the patch up for WITH clause support, because
that's true, but it's also beside the point.
--
Peter Geoghegan
explained, at a minimum.
Even if I'm completely wrong about there being a security hazard,
maybe the suggestion that there might be still gives you some idea of
what I mean about unintended consequences.
--
Peter Geoghegan
ry to block it, but I do have misgivings about your
timeframe.
--
Peter Geoghegan
TEs for the target
relation? That's certainly why we always see unaliased target table
"m" with the alias "m_1" in EXPLAIN output, so I would not be
surprised if it caused another EXPLAIN issue.
--
Peter Geoghegan
From c2aaea9d7e87ffa6075408f61c0748d31d61312a Mon Sep 17 00:00:00 2
On Fri, Mar 23, 2018 at 11:15 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> I agree that this is very similar, as far as the RTEs go. What is
> dissimilar is the fact that there is hard-coded knowledge of both
> through parsing, planning, and execution. It's everything, t
by 5f173040 cannot recur here is one concrete thing you could
do. Documenting/explaining the ri_RangeTableIndex/ri_mergeTargetRTI
divide is another. The comment above ri_mergeTargetRTI is totally
inadequate.
--
Peter Geoghegan
), just like ParallelQueryMain().
I'll come up with a patch soon.
--
Peter Geoghegan
On Thu, Mar 1, 2018 at 1:04 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Patch looks fine to me, will push.
Thank you.
--
Peter Geoghegan
unced way too many times already. I really hope it
doesn't fall through the cracks for v11.
> - Faster inserts with mostly-monotonically increasing values
>
> NR, patch from Dec, CF entry created Feb.
>
> Nice performance improvement. Hasn't gotten that much review, and it
>
ich are comments (there is also another 300
lines for the Bloom filter implementation). The changes to the
IndexBuildHeapScan() interface made by parallel CREATE INDEX allowed
me to simplify things considerably in the most recent revision.
--
Peter Geoghegan
The only tests possible with stock Postgres are performed by CREATE
INDEX/REINDEX. That generally isn't practical as a general smoke test,
not least because you cannot do a CREATE INDEX on a standby -- unlike
the amcheck heapallindexed test.
--
Peter Geoghegan
301 - 400 of 3106 matches
Mail list logo
