Patch: Write Amplification Reduction Method (WARM)

On Wed, Aug 31, 2016 at 1:45 PM, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

We discussed a few ideas to address the "Duplicate Scan" problem. For
example, we can teach Index AMs to discard any duplicate (key, CTID) insert
requests. Or we could guarantee uniqueness by either only allowing updates
in one lexical order. While the former is a more complete solution to avoid
duplicate entries, searching through large number of keys for non-unique
indexes could be a drag on performance. The latter approach may not be
sufficient for many workloads. Also tracking increment/decrement for many
indexes will be non-trivial.

There is another problem with allowing many index entries pointing to the
same WARM chain. It will be non-trivial to know how many index entries are
currently pointing to the WARM chain and index/heap vacuum will throw up
more challenges.

Instead, what I would like to propose and the patch currently implements
is to restrict WARM update to once per chain. So the first non-HOT update
to a tuple or a HOT chain can be a WARM update. The chain can further be
HOT updated any number of times. But it can no further be WARM updated.
This might look too restrictive, but it can still bring down the number of
regular updates by almost 50%. Further, if we devise a strategy to convert
a WARM chain back to HOT chain, it can again be WARM updated. (This part is
currently not implemented). A good side effect of this simple strategy is
that we know there can maximum two index entries pointing to any given WARM
chain.

We should probably think about coordinating with my btree patch.

From the description above, the strategy is quite readily "upgradable" to
one in which the indexam discards duplicate (key,ctid) pairs and that would
remove the limitation of only one WARM update... right?

On Wed, Aug 31, 2016 at 10:38 PM, Claudio Freire <klaussfreire@gmail.com>
wrote:

On Wed, Aug 31, 2016 at 1:45 PM, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

We discussed a few ideas to address the "Duplicate Scan" problem. For
example, we can teach Index AMs to discard any duplicate (key, CTID) insert
requests. Or we could guarantee uniqueness by either only allowing updates
in one lexical order. While the former is a more complete solution to avoid
duplicate entries, searching through large number of keys for non-unique
indexes could be a drag on performance. The latter approach may not be
sufficient for many workloads. Also tracking increment/decrement for many
indexes will be non-trivial.

There is another problem with allowing many index entries pointing to the
same WARM chain. It will be non-trivial to know how many index entries are
currently pointing to the WARM chain and index/heap vacuum will throw up
more challenges.

Instead, what I would like to propose and the patch currently implements
is to restrict WARM update to once per chain. So the first non-HOT update
to a tuple or a HOT chain can be a WARM update. The chain can further be
HOT updated any number of times. But it can no further be WARM updated.
This might look too restrictive, but it can still bring down the number of
regular updates by almost 50%. Further, if we devise a strategy to convert
a WARM chain back to HOT chain, it can again be WARM updated. (This part is
currently not implemented). A good side effect of this simple strategy is
that we know there can maximum two index entries pointing to any given WARM
chain.

We should probably think about coordinating with my btree patch.

From the description above, the strategy is quite readily "upgradable" to
one in which the indexam discards duplicate (key,ctid) pairs and that would
remove the limitation of only one WARM update... right?

Yes, we should be able to add further optimisations on lines you're working
on, but what I like about the current approach is that a) it reduces
complexity of the patch and b) having thought about cleaning up WARM
chains, limiting number of index entries per root chain to a small number
will simplify that aspect too.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Aug 31, 2016 at 10:15 PM, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

Hi All,

As previously discussed [1], WARM is a technique to reduce write
amplification when an indexed column of a table is updated. HOT fails to
handle such updates and ends up inserting a new index entry in all indexes
of the table, irrespective of whether the index key has changed or not for
a specific index. The problem was highlighted by Uber's blog post [2], but
it was a well known problem and affects many workloads.

I realised that the patches were bit-rotten because of 8e1e3f958fb. Rebased
patches on the current master are attached. I also took this opportunity to
correct some white space errors and improve formatting of the README.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Aug 31, 2016 at 10:15:33PM +0530, Pavan Deolasee wrote:

Instead, what I would like to propose and the patch currently implements is to
restrict WARM update to once per chain. So the first non-HOT update to a tuple
or a HOT chain can be a WARM update. The chain can further be HOT updated any
number of times. But it can no further be WARM updated. This might look too
restrictive, but it can still bring down the number of regular updates by
almost 50%. Further, if we devise a strategy to convert a WARM chain back to
HOT chain, it can again be WARM updated. (This part is currently not
implemented). A good side effect of this simple strategy is that we know there
can maximum two index entries pointing to any given WARM chain.

I like the simplified approach, as long as it doesn't block further
improvements.

Headline TPS numbers:

Master:

transaction type: update.sql
scaling factor: 700
query mode: simple
number of clients: 16
number of threads: 8
duration: 57600 s
number of transactions actually processed: 65552986
latency average: 14.059 ms
tps = 1138.072117 (including connections establishing)
tps = 1138.072156 (excluding connections establishing)

WARM:

transaction type: update.sql
scaling factor: 700
query mode: simple
number of clients: 16
number of threads: 8
duration: 57600 s
number of transactions actually processed: 116168454
latency average: 7.933 ms
tps = 2016.812924 (including connections establishing)
tps = 2016.812997 (excluding connections establishing)

These are very impressive results.

Converting WARM chains back to HOT chains (VACUUM ?)
---------------------------------------------------------------------------------

The current implementation of WARM allows only one WARM update per chain. This
simplifies the design and addresses certain issues around duplicate scans. But
this also implies that the benefit of WARM will be no more than 50%, which is
still significant, but if we could return WARM chains back to normal status, we
could do far more WARM updates.

A distinct property of a WARM chain is that at least one index has more than
one live index entries pointing to the root of the chain. In other words, if we
can remove duplicate entry from every index or conclusively prove that there
are no duplicate index entries for the root line pointer, the chain can again
be marked as HOT.

I had not thought of how to convert from WARM to HOT yet.

Here is one idea, but more thoughts/suggestions are most welcome. 

A WARM chain has two parts, separated by the tuple that caused WARM update. All
tuples in each part has matching index keys, but certain index keys may not
match between these two parts. Lets say we mark heap tuples in each part with a
special Red-Blue flag. The same flag is replicated in the index tuples. For
example, when new rows are inserted in a table, they are marked with Blue flag
and the index entries associated with those rows are also marked with Blue
flag. When a row is WARM updated, the new version is marked with Red flag and
the new index entry created by the update is also marked with Red flag.

Heap chain: lp  [1] [2] [3] [4]
  [aaaa, 1111]B -> [aaaa, 1111]B -> [bbbb, 1111]R -> [bbbb, 1111]R

Index1: (aaaa)B points to 1 (satisfies only tuples marked with B)
(bbbb)R points to 1 (satisfies only tuples marked with R)

Index2: (1111)B points to 1 (satisfies both B and R tuples)

It's clear that for indexes with Red and Blue pointers, a heap tuple with Blue
flag will be reachable from Blue pointer and that with Red flag will be
reachable from Red pointer. But for indexes which did not create a new entry,
both Blue and Red tuples will be reachable from Blue pointer (there is no Red
pointer in such indexes). So, as a side note, matching Red and Blue flags is
not enough from index scan perspective.

During first heap scan of VACUUM, we look for tuples with HEAP_WARM_TUPLE set.
If all live tuples in the chain are either marked with Blue flag or Red flag
(but no mix of Red and Blue), then the chain is a candidate for HOT conversion.

Uh, if the chain is all blue, then there is are WARM entries so it
already a HOT chain, so there is nothing to do, right?

We remember the root line pointer and Red-Blue flag of the WARM chain in a
separate array.

If we have a Red WARM chain, then our goal is to remove Blue pointers and vice
versa. But there is a catch. For Index2 above, there is only Blue pointer
and that must not be removed. IOW we should remove Blue pointer iff a Red
pointer exists. Since index vacuum may visit Red and Blue pointers in any
order, I think we will need another index pass to remove dead
index pointers. So in the first index pass we check which WARM candidates have
2 index pointers. In the second pass, we remove the dead pointer and reset Red
flag is the surviving index pointer is Red.

Why not just remember the tid of chains converted from WARM to HOT, then
use "amrecheck" on an index entry matching that tid to see if the index
matches one of the entries in the chain. (It will match all of them or
none of them, because they are all red.) I don't see a point in
coloring the index entries as reds as later you would have to convert to
blue in the WARM-to-HOT conversion, and a vacuum crash could lead to
inconsistencies. Consider that you can just call "amrecheck" on the few
chains that have converted from WARM to HOT. I believe this is more
crash-safe too. However, if you have converted WARM to HOT in the heap,
but crash durin the index entry removal, you could potentially have
duplicates in the index later, which is bad.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+                     Ancient Roman grave inscription +

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

On Wed, Aug 31, 2016 at 04:03:29PM -0400, Bruce Momjian wrote:

Why not just remember the tid of chains converted from WARM to HOT, then
use "amrecheck" on an index entry matching that tid to see if the index
matches one of the entries in the chain. (It will match all of them or
none of them, because they are all red.) I don't see a point in
coloring the index entries as reds as later you would have to convert to
blue in the WARM-to-HOT conversion, and a vacuum crash could lead to
inconsistencies. Consider that you can just call "amrecheck" on the few
chains that have converted from WARM to HOT. I believe this is more
crash-safe too. However, if you have converted WARM to HOT in the heap,
but crash during the index entry removal, you could potentially have
duplicates in the index later, which is bad.

I think Pavan had the "crash durin the index entry removal" fixed via:

During the second heap scan, we fix WARM chain by clearing HEAP_WARM_TUPLE flag
and also reset Red flag to Blue.

so the marking from WARM to HOT only happens after the index has been cleaned.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+                     Ancient Roman grave inscription +

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

On Thu, Sep 1, 2016 at 1:33 AM, Bruce Momjian <bruce@momjian.us> wrote:

On Wed, Aug 31, 2016 at 10:15:33PM +0530, Pavan Deolasee wrote:

Instead, what I would like to propose and the patch currently implements

is to

restrict WARM update to once per chain. So the first non-HOT update to a

tuple

or a HOT chain can be a WARM update. The chain can further be HOT

updated any

number of times. But it can no further be WARM updated. This might look

too

restrictive, but it can still bring down the number of regular updates by
almost 50%. Further, if we devise a strategy to convert a WARM chain

back to

HOT chain, it can again be WARM updated. (This part is currently not
implemented). A good side effect of this simple strategy is that we know

there

can maximum two index entries pointing to any given WARM chain.

I like the simplified approach, as long as it doesn't block further
improvements.

Yes, the proposed approach is simple yet does not stop us from improving
things further. Moreover it has shown good performance characteristics and
I believe it's a good first step.

Master:
tps = 1138.072117 (including connections establishing)

WARM:
tps = 2016.812924 (including connections establishing)

These are very impressive results.

Thanks. What's also interesting and something that headline numbers don't
show is that WARM TPS is as much as 3 times of master TPS when the
percentage of WARM updates is very high. Notice the spike in TPS in the
comparison graph.

Results with non-default heap fill factor are even better. In both cases,
the improvement in TPS stays constant over long periods.

During first heap scan of VACUUM, we look for tuples with

HEAP_WARM_TUPLE set.

If all live tuples in the chain are either marked with Blue flag or Red

flag

(but no mix of Red and Blue), then the chain is a candidate for HOT

conversion.

Uh, if the chain is all blue, then there is are WARM entries so it
already a HOT chain, so there is nothing to do, right?

For aborted WARM updates, the heap chain may be all blue, but there may
still be a red index pointer which must be cleared before we allow further
WARM updates to the chain.

We remember the root line pointer and Red-Blue flag of the WARM chain in

a

separate array.

If we have a Red WARM chain, then our goal is to remove Blue pointers

and vice

versa. But there is a catch. For Index2 above, there is only Blue pointer
and that must not be removed. IOW we should remove Blue pointer iff a Red
pointer exists. Since index vacuum may visit Red and Blue pointers in any
order, I think we will need another index pass to remove dead
index pointers. So in the first index pass we check which WARM

candidates have

2 index pointers. In the second pass, we remove the dead pointer and

reset Red

flag is the surviving index pointer is Red.

Why not just remember the tid of chains converted from WARM to HOT, then
use "amrecheck" on an index entry matching that tid to see if the index
matches one of the entries in the chain.

That will require random access to heap during index vacuum phase,
something I would like to avoid. But we can have that as a fall back
solution for handling aborted vacuums.

(It will match all of them or
none of them, because they are all red.) I don't see a point in
coloring the index entries as reds as later you would have to convert to
blue in the WARM-to-HOT conversion, and a vacuum crash could lead to
inconsistencies.

Yes, that's a concern since the conversion of red to blue will also need to
WAL logged to ensure that a crash doesn't leave us in inconsistent state. I
still think that this will be an overall improvement as compared to
allowing one WARM update per chain.

Consider that you can just call "amrecheck" on the few
chains that have converted from WARM to HOT. I believe this is more
crash-safe too. However, if you have converted WARM to HOT in the heap,
but crash durin the index entry removal, you could potentially have
duplicates in the index later, which is bad.

As you probably already noted, we clear heap flags only after all indexes
are cleared of duplicate entries and hence a crash in between should not
cause any correctness issue. As long as heap tuples are marked as warm,
amrecheck will ensure that only valid tuples are returned to the caller.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Sep 1, 2016 at 02:37:40PM +0530, Pavan Deolasee wrote:

I like the simplified approach, as long as it doesn't block further
improvements.

Yes, the proposed approach is simple yet does not stop us from improving things
further. Moreover it has shown good performance characteristics and I believe
it's a good first step.

Agreed. This is BIG. Do you think it can be done for PG 10?

Thanks. What's also interesting and something that headline numbers don't show
is that WARM TPS is as much as 3 times of master TPS when the percentage of
WARM updates is very high. Notice the spike in TPS in the comparison graph.

Results with non-default heap fill factor are even better. In both cases, the
improvement in TPS stays constant over long periods. 

Yes, I expect the benefits of this to show up in better long-term
performance.

During first heap scan of VACUUM, we look for tuples with HEAP_WARM_TUPLE

set.

If all live tuples in the chain are either marked with Blue flag or Red

flag

(but no mix of Red and Blue), then the chain is a candidate for HOT

conversion.

Uh, if the chain is all blue, then there is are WARM entries so it
already a HOT chain, so there is nothing to do, right?

For aborted WARM updates, the heap chain may be all blue, but there may still
be a red index pointer which must be cleared before we allow further WARM
updates to the chain.

Ah, understood now. Thanks.

Why not just remember the tid of chains converted from WARM to HOT, then
use "amrecheck" on an index entry matching that tid to see if the index
matches one of the entries in the chain. 

That will require random access to heap during index vacuum phase, something I
would like to avoid. But we can have that as a fall back solution for handling
aborted vacuums. 

Yes, that is true. So the challenge is figuring how which of the index
entries pointing to the same tid is valid, and coloring helps with that?

(It will match all of them or
none of them, because they are all red.)  I don't see a point in
coloring the index entries as reds as later you would have to convert to
blue in the WARM-to-HOT conversion, and a vacuum crash could lead to
inconsistencies. 

Yes, that's a concern since the conversion of red to blue will also need to WAL
logged to ensure that a crash doesn't leave us in inconsistent state. I still
think that this will be an overall improvement as compared to allowing one WARM
update per chain.

OK. I will think some more on this to see if I can come with another
approach.

 

Consider that you can just call "amrecheck" on the few
chains that have converted from WARM to HOT.  I believe this is more
crash-safe too.  However, if you have converted WARM to HOT in the heap,
but crash durin the index entry removal, you could potentially have
duplicates in the index later, which is bad.

As you probably already noted, we clear heap flags only after all indexes are
cleared of duplicate entries and hence a crash in between should not cause any
correctness issue. As long as heap tuples are marked as warm, amrecheck will
ensure that only valid tuples are returned to the caller.

OK, got it.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+                     Ancient Roman grave inscription +

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

On Thu, Sep 1, 2016 at 9:44 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Thu, Sep 1, 2016 at 02:37:40PM +0530, Pavan Deolasee wrote:

I like the simplified approach, as long as it doesn't block further
improvements.

Yes, the proposed approach is simple yet does not stop us from improving

things

further. Moreover it has shown good performance characteristics and I

believe

it's a good first step.

Agreed. This is BIG. Do you think it can be done for PG 10?

I definitely think so. The patches as submitted are fully functional and
sufficient. Of course, there are XXX and TODOs that I hope to sort out
during the review process. There are also further tests needed to ensure
that the feature does not cause significant regression in the worst cases.
Again something I'm willing to do once I get some feedback on the broader
design and test cases. What I am looking at this stage is to know if I've
missed something important in terms of design or if there is some show
stopper that I overlooked.

Latest patches rebased with current master are attached. I also added a few
more comments to the code. I forgot to give a brief about the patches, so
including that as well.

0001_track_root_lp_v4.patch: This patch uses a free t_infomask2 bit to
track latest tuple in an update chain. The t_ctid.ip_posid is used to track
the root line pointer of the update chain. We do this only in the latest
tuple in the chain because most often that tuple will be updated and we
need to quickly find the root only during update.

0002_warm_updates_v4.patch: This patch implements the core of WARM logic.
During WARM update, we only insert new entries in the indexes whose key has
changed. But instead of indexing the real TID of the new tuple, we index
the root line pointer and then use additional recheck logic to ensure only
correct tuples are returned from such potentially broken HOT chains. Each
index AM must implement a amrecheck method to support WARM. The patch
currently implements this for hash and btree indexes.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Sep 5, 2016 at 1:53 PM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

0001_track_root_lp_v4.patch: This patch uses a free t_infomask2 bit to track
latest tuple in an update chain. The t_ctid.ip_posid is used to track the
root line pointer of the update chain. We do this only in the latest tuple
in the chain because most often that tuple will be updated and we need to
quickly find the root only during update.

0002_warm_updates_v4.patch: This patch implements the core of WARM logic.
During WARM update, we only insert new entries in the indexes whose key has
changed. But instead of indexing the real TID of the new tuple, we index the
root line pointer and then use additional recheck logic to ensure only
correct tuples are returned from such potentially broken HOT chains. Each
index AM must implement a amrecheck method to support WARM. The patch
currently implements this for hash and btree indexes.

Moved to next CF, I was surprised to see that it is not *that* large:
43 files changed, 1539 insertions(+), 199 deletions(-)
--
Michael

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

On 09/05/2016 06:53 AM, Pavan Deolasee wrote:

On Thu, Sep 1, 2016 at 9:44 PM, Bruce Momjian <bruce@momjian.us
<mailto:bruce@momjian.us>> wrote:

On Thu, Sep 1, 2016 at 02:37:40PM +0530, Pavan Deolasee wrote:

I like the simplified approach, as long as it doesn't block further
improvements.

Yes, the proposed approach is simple yet does not stop us from improving things
further. Moreover it has shown good performance characteristics and I believe
it's a good first step.

Agreed. This is BIG. Do you think it can be done for PG 10?

I definitely think so. The patches as submitted are fully functional and
sufficient. Of course, there are XXX and TODOs that I hope to sort out
during the review process. There are also further tests needed to ensure
that the feature does not cause significant regression in the worst
cases. Again something I'm willing to do once I get some feedback on the
broader design and test cases. What I am looking at this stage is to
know if I've missed something important in terms of design or if there
is some show stopper that I overlooked.

Latest patches rebased with current master are attached. I also added a
few more comments to the code. I forgot to give a brief about the
patches, so including that as well.

0001_track_root_lp_v4.patch: This patch uses a free t_infomask2 bit to
track latest tuple in an update chain. The t_ctid.ip_posid is used to
track the root line pointer of the update chain. We do this only in the
latest tuple in the chain because most often that tuple will be updated
and we need to quickly find the root only during update.

0002_warm_updates_v4.patch: This patch implements the core of WARM
logic. During WARM update, we only insert new entries in the indexes
whose key has changed. But instead of indexing the real TID of the new
tuple, we index the root line pointer and then use additional recheck
logic to ensure only correct tuples are returned from such potentially
broken HOT chains. Each index AM must implement a amrecheck method to
support WARM. The patch currently implements this for hash and btree
indexes.

Hi,

I've been looking at the patch over the past few days, running a bunch
of benchmarks etc. I can confirm the significant speedup, often by more
than 75% (depending on number of indexes, whether the data set fits into
RAM, etc.). Similarly for the amount of WAL generated, although that's a
bit more difficult to evaluate due to full_page_writes.

I'm not going to send detailed results, as that probably does not make
much sense at this stage of the development - I can repeat the tests
once the open questions get resolved.

There's a lot of useful and important feedback in the thread(s) so far,
particularly the descriptions of various failure cases. I think it'd be
very useful to collect those examples and turn them into regression
tests - that's something the patch should include anyway.

I don't really have much comments regarding the code, but during the
testing I noticed a bit strange behavior when updating statistics.
Consider a table like this:

create table t (a int, b int, c int) with (fillfactor = 10);
insert into t select i, i from generate_series(1,1000) s(i);
create index on t(a);
create index on t(b);

and update:

update t set a = a+1, b=b+1;

which has to update all indexes on the table, but:

select n_tup_upd, n_tup_hot_upd from pg_stat_user_tables

n_tup_upd | n_tup_hot_upd
-----------+---------------
1000 | 1000

So it's still counted as "WARM" - does it make sense? I mean, we're
creating a WARM chain on the page, yet we have to add pointers into all
indexes (so not really saving anything). Doesn't this waste the one WARM
update per HOT chain without actually getting anything in return?

The way this is piggy-backed on the current HOT statistics seems a bit
strange for another reason, although WARM is a relaxed version of HOT.
Until now, HOT was "all or nothing" - we've either added index entries
to all indexes or none of them. So the n_tup_hot_upd was fine.

But WARM changes that - it allows adding index entries only to a subset
of indexes, which means the "per row" n_tup_hot_upd counter is not
sufficient. When you have a table with 10 indexes, and the counter
increases by 1, does that mean the update added index tuple to 1 index
or 9 of them?

So I think we'll need two counters to track WARM - number of index
tuples we've added, and number of index tuples we've skipped. So
something like blks_hit and blks_read. I'm not sure whether we should
replace the n_tup_hot_upd entirely, or keep it for backwards
compatibility (and to track perfectly HOT updates).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, Oct 5, 2016 at 1:43 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:

I've been looking at the patch over the past few days, running a bunch of
benchmarks etc.

Thanks for doing that.

I can confirm the significant speedup, often by more than 75% (depending
on number of indexes, whether the data set fits into RAM, etc.). Similarly
for the amount of WAL generated, although that's a bit more difficult to
evaluate due to full_page_writes.

I'm not going to send detailed results, as that probably does not make
much sense at this stage of the development - I can repeat the tests once
the open questions get resolved.

Sure. Anything that stands out? Any regression that you see? I'm not sure
if your benchmarks exercise the paths which might show overheads without
any tangible benefits. For example, I wonder if a test with many indexes
where most of them get updated and then querying the table via those
updated indexes could be one such test case.

There's a lot of useful and important feedback in the thread(s) so far,
particularly the descriptions of various failure cases. I think it'd be
very useful to collect those examples and turn them into regression tests -
that's something the patch should include anyway.

Sure. I added only a handful test cases which I knew regression isn't
covering. But I'll write more of them. One good thing is that the code gets
heavily exercised even during regression. I caught and fixed multiple bugs
running regression. I'm not saying that's enough, but it certainly gives
some confidence.

and update:

update t set a = a+1, b=b+1;

which has to update all indexes on the table, but:

select n_tup_upd, n_tup_hot_upd from pg_stat_user_tables

n_tup_upd | n_tup_hot_upd
-----------+---------------
1000 | 1000

So it's still counted as "WARM" - does it make sense?

No, it does not. The code currently just marks any update as a WARM update
if the table supports it and there is enough free space in the page. And
yes, you're right. It's worth fixing that because of one-WARM update per
chain limitation. Will fix.

The way this is piggy-backed on the current HOT statistics seems a bit
strange for another reason,

Agree. We could add a similar n_tup_warm_upd counter.

But WARM changes that - it allows adding index entries only to a subset of
indexes, which means the "per row" n_tup_hot_upd counter is not sufficient.
When you have a table with 10 indexes, and the counter increases by 1, does
that mean the update added index tuple to 1 index or 9 of them?

How about having counters similar to n_tup_ins/n_tup_del for indexes as
well? Today it does not make sense because every index gets the same number
of inserts, but WARM will change that.

For example, we could have idx_tup_insert and idx_tup_delete that shows up
in pg_stat_user_indexes. I don't know if idx_tup_delete adds any value, but
one can then look at idx_tup_insert for various indexes to get a sense
which indexes receives more inserts than others. The indexes which receive
more inserts are the ones being frequently updated as compared to other
indexes.

This also relates to vacuuming strategies. Today HOT updates do not count
for triggering vacuum (or to be more precise, HOT pruned tuples are
discounted while counting dead tuples). WARM tuples get the same treatment
as far as pruning is concerned, but since they cause fresh index inserts, I
wonder if we need some mechanism to cleanup the dead line pointers and dead
index entries. This will become more important if we do something to
convert WARM chains into HOT chains, something that only VACUUM can do in
the design I've proposed so far.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10/06/2016 07:36 AM, Pavan Deolasee wrote:

On Wed, Oct 5, 2016 at 1:43 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote:

...

I can confirm the significant speedup, often by more than 75%
(depending on number of indexes, whether the data set fits into RAM,
etc.). Similarly for the amount of WAL generated, although that's a
bit more difficult to evaluate due to full_page_writes.

I'm not going to send detailed results, as that probably does not
make much sense at this stage of the development - I can repeat the
tests once the open questions get resolved.

Sure. Anything that stands out? Any regression that you see? I'm not
sure if your benchmarks exercise the paths which might show overheads
without any tangible benefits. For example, I wonder if a test with many
indexes where most of them get updated and then querying the table via
those updated indexes could be one such test case.

No, nothing that would stand out. Let me explain what benchmark(s) I've
done. I've made some minor mistakes when running the benchmarks, so I
plan to rerun them and post the results after that. So let's take the
data with a grain of salt.

My goal was to compare current non-HOT behavior (updating all indexes)
with the WARM (updating only indexes on modified columns), and I've
taken two approaches:

1) fixed number of indexes, update variable number of columns

Create a table with 8 secondary indexes and then run a bunch of
benchmarks updating increasing number of columns. So the first run did

UPDATE t SET c1 = c1+1 WHERE id = :id;

while the second did

UPDATE t SET c1 = c1+1, c2 = c2+1 WHERE id = :id;

and so on, up to updating all the columns in the last run. I've used
multiple scripts to update all the columns / indexes uniformly
(essentially using multiple "-f" flags with pgbench). The runs were
fairly long (2h, enough to get stable behavior).

For a small data set (fits into RAM), the results look like this:

master patched diff
1 5994 8490 +42%
2 4347 7903 +81%
3 4340 7400 +70%
4 4324 6929 +60%
5 4256 6495 +52%
6 4253 5059 +19%
7 4235 4534 +7%
8 4194 4237 +1%

and the amount of WAL generated (after correction for tps difference)
looks like this (numbers are MBs)

master patched diff
1 27257 18508 -32%
2 21753 14599 -33%
3 21912 15864 -28%
4 22021 17135 -22%
5 21819 18258 -16%
6 21929 20659 -6%
7 21994 22234 +1%
8 21851 23267 +6%

So this is quite significant difference. I'm pretty sure the minor WAL
increase for the last two runs is due to full page writes (which also
affects the preceding runs, making the WAL reduction smaller than the
tps increase).

I do have results for larger data sets (>RAM), the results are very
similar although the speedup seems a bit smaller. But I need to rerun those.

2) single-row update, adding indexes between runs

This is kinda the opposite of the previous approach, i.e. transactions
always update a single column (multiple scripts to update the columns
uniformly), but there are new indexes added between runs. The results
(for a large data set, exceeding RAM) look like this:

master patched diff
0 954 1404 +47%
1 701 1045 +49%
2 484 816 +70%
3 346 683 +97%
4 248 608 +145%
5 190 525 +176%
6 152 397 +161%
7 123 315 +156%
8 123 270 +119%

So this looks really interesting.

There's a lot of useful and important feedback in the thread(s) so
far, particularly the descriptions of various failure cases. I think
it'd be very useful to collect those examples and turn them into
regression tests - that's something the patch should include anyway.

Sure. I added only a handful test cases which I knew regression isn't
covering. But I'll write more of them. One good thing is that the code
gets heavily exercised even during regression. I caught and fixed
multiple bugs running regression. I'm not saying that's enough, but it
certainly gives some confidence.

I don't see any changes to src/test in the patch, so I'm not sure what
you mean when you say you added a handful of test cases?

and update:

update t set a = a+1, b=b+1;

which has to update all indexes on the table, but:

select n_tup_upd, n_tup_hot_upd from pg_stat_user_tables

n_tup_upd | n_tup_hot_upd
-----------+---------------
1000 | 1000

So it's still counted as "WARM" - does it make sense?

No, it does not. The code currently just marks any update as a WARM
update if the table supports it and there is enough free space in the
page. And yes, you're right. It's worth fixing that because of one-WARM
update per chain limitation. Will fix.

Hmmm, so this makes monitoring of %WARM during benchmarks less reliable
than I hoped for :-(

The way this is piggy-backed on the current HOT statistics seems a
bit strange for another reason,

Agree. We could add a similar n_tup_warm_upd counter.

Yes, although HOT is a special case of WARM. But it probably makes sense
to differentiate them, I guess.

But WARM changes that - it allows adding index entries only to a
subset of indexes, which means the "per row" n_tup_hot_upd counter
is not sufficient. When you have a table with 10 indexes, and the
counter increases by 1, does that mean the update added index tuple
to 1 index or 9 of them?

How about having counters similar to n_tup_ins/n_tup_del for indexes
as well? Today it does not make sense because every index gets the
same number of inserts, but WARM will change that.

For example, we could have idx_tup_insert and idx_tup_delete that shows
up in pg_stat_user_indexes. I don't know if idx_tup_delete adds any
value, but one can then look at idx_tup_insert for various indexes to
get a sense which indexes receives more inserts than others. The indexes
which receive more inserts are the ones being frequently updated as
compared to other indexes.

Hmmm, I'm not sure that'll work. I mean, those metrics would be useful
(although I can't think of a use case for idx_tup_delete), but I'm not
sure it's a enough to measure WARM. We need to compute

index_tuples_inserted / index_tuples_total

where (index_tuples_total - index_tuples_inserted) is the number of
index tuples we've been able to skip thanks to WARM. So we'd also need
to track the number of index tuples that we skipped for the index, and
I'm not sure that's a good idea.

Also, we really don't care about inserted tuples - what matters for WARM
are updates, so idx_tup_insert is either useless (because it also
includes non-UPDATE entries) or the naming is misleading.

This also relates to vacuuming strategies. Today HOT updates do not
count for triggering vacuum (or to be more precise, HOT pruned tuples
are discounted while counting dead tuples). WARM tuples get the same
treatment as far as pruning is concerned, but since they cause fresh
index inserts, I wonder if we need some mechanism to cleanup the dead
line pointers and dead index entries. This will become more important if
we do something to convert WARM chains into HOT chains, something that
only VACUUM can do in the design I've proposed so far.

True.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Thanks for the patch. This shows a very good performance improvement.

I started reviewing the patch, during this process and I ran the regression
test on the WARM patch. I observed a failure in create_index test.
This may be a bug in code or expected that needs to be corrected.

Regards,
Hari Babu
Fujitsu Australia

On Tue, Nov 8, 2016 at 9:13 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

Thanks for the patch. This shows a very good performance improvement.

Thank you. Can you please share the benchmark you ran, results and
observations?

I started reviewing the patch, during this process and I ran the regression
test on the WARM patch. I observed a failure in create_index test.
This may be a bug in code or expected that needs to be corrected.

Can you please share the diff? I ran regression after applying patch on the
current master and did not find any change? Does it happen consistently?

I'm also attaching fresh set of patches. The first patch hasn't changed at
all (though I changed the name to v5 to keep it consistent with the other
patch). The second patch has the following changes:

1. WARM updates are now tracked separately. We still don't count number of
index inserts separately as suggested by Tomas.
2. We don't do a WARM update if all columns referenced by all indexes have
changed. Ideally, we should check if all indexes will require an update and
avoid WARM. So there is still some room for improvement here
3. I added a very minimal regression test case. But really, it just
contains one test case which I specifically wanted to test.

So not a whole lot of changes since the last version. I'm still waiting for
some serious review of the design/code before I spend a lot more time on
the patch. I hope the patch receives some attention in this CF.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Nov 12, 2016 at 10:12 PM, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

On Tue, Nov 8, 2016 at 9:13 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

Thanks for the patch. This shows a very good performance improvement.

Thank you. Can you please share the benchmark you ran, results and
observations?

I just ran a performance test on my laptop with minimal configuration, it
didn't show much
improvement, currently I don't have access to a big machine to test the
performance.

I started reviewing the patch, during this process and I ran the regression

test on the WARM patch. I observed a failure in create_index test.
This may be a bug in code or expected that needs to be corrected.

Can you please share the diff? I ran regression after applying patch on
the current master and did not find any change? Does it happen consistently?

Yes, it is happening consistently. I ran the make installcheck. Attached
the regression.diffs file with the failed test.
I applied the previous warm patch on this commit -
e3e66d8a9813d22c2aa027d8f373a96d4d4c1b15

Regards,
Hari Babu
Fujitsu Australia

On Tue, Nov 15, 2016 at 5:58 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

On Sat, Nov 12, 2016 at 10:12 PM, Pavan Deolasee <pavan.deolasee@gmail.com

wrote:

On Tue, Nov 8, 2016 at 9:13 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

Thanks for the patch. This shows a very good performance improvement.

Thank you. Can you please share the benchmark you ran, results and
observations?

I just ran a performance test on my laptop with minimal configuration, it
didn't show much
improvement, currently I don't have access to a big machine to test the
performance.

I started reviewing the patch, during this process and I ran the

regression
test on the WARM patch. I observed a failure in create_index test.
This may be a bug in code or expected that needs to be corrected.

Can you please share the diff? I ran regression after applying patch on
the current master and did not find any change? Does it happen consistently?

Yes, it is happening consistently. I ran the make installcheck. Attached
the regression.diffs file with the failed test.
I applied the previous warm patch on this commit
- e3e66d8a9813d22c2aa027d8f373a96d4d4c1b15

Are you able to reproduce the issue?

Currently the patch is moved to next CF with "needs review" state.

Regards,
Hari Babu
Fujitsu Australia

On Fri, Dec 2, 2016 at 8:34 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

On Tue, Nov 15, 2016 at 5:58 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

Yes, it is happening consistently. I ran the make installcheck. Attached
the regression.diffs file with the failed test.
I applied the previous warm patch on this commit
- e3e66d8a9813d22c2aa027d8f373a96d4d4c1b15

Are you able to reproduce the issue?

Apologies for the delay. I could reproduce this on a different environment.
It was a case of uninitialised variable and hence the inconsistent results.

I've updated the patches after fixing the issue. Multiple rounds of
regression passes for me without any issue. Please let me know if it works
for you.

Currently the patch is moved to next CF with "needs review" state.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

I noticed that this patch changes HeapSatisfiesHOTAndKeyUpdate() by
adding one more set of attributes to check, and one more output boolean
flag. My patch to add indirect indexes also modifies that routine to
add the same set of things. I think after committing both these
patches, the API is going to be fairly ridiculous. I propose to use a
different approach.

With your WARM and my indirect indexes, plus the additions for for-key
locks, plus identity columns, there is no longer a real expectation that
we can exit early from the function. In your patch, as well as mine,
there is a semblance of optimization that tries to avoid computing the
updated_attrs output bitmapset if the pointer is not passed in, but it's
effectively pointless because the only interesting use case is from
ExecUpdate() which always activates the feature. Can we just agree to
drop that?

If we do drop that, then the function can become much simpler: compare
all columns in new vs. old, return output bitmapset of changed columns.
Then "satisfies_hot" and all the other boolean output flags can be
computed simply in the caller by doing bms_overlap().

Thoughts?

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Alvaro Herrera wrote:

With your WARM and my indirect indexes, plus the additions for for-key
locks, plus identity columns, there is no longer a real expectation that
we can exit early from the function. In your patch, as well as mine,
there is a semblance of optimization that tries to avoid computing the
updated_attrs output bitmapset if the pointer is not passed in, but it's
effectively pointless because the only interesting use case is from
ExecUpdate() which always activates the feature. Can we just agree to
drop that?

I think the only case that gets worse is the path that does
simple_heap_update, which is used for DDL. I would be very surprised if
a change there is noticeable, when compared to the rest of the stuff
that goes on for DDL commands.

Now, after saying that, I think that a table with a very large number of
columns is going to be affected by this. But we don't really need to
compute the output bits for every single column -- we only care about
those that are covered by some index. So we should pass an input
bitmapset comprising all such columns, and the output bitmapset only
considers those columns, and ignores columns not indexed. My patch for
indirect indexes already does something similar (though it passes a
bitmapset of columns indexed by indirect indexes only, so it needs a
tweak there.)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 2 December 2016 at 07:36, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

I've updated the patches after fixing the issue. Multiple rounds of
regression passes for me without any issue. Please let me know if it works
for you.

Hi Pavan,

Today i was playing with your patch and running some tests and found
some problems i wanted to report before i forget them ;)

* You need to add a prototype in src/backend/utils/adt/pgstatfuncs.c:
extern Datum pg_stat_get_tuples_warm_updated(PG_FUNCTION_ARGS);

* The isolation test for partial_index fails (attached the regression.diffs)

* running a home-made test i have at hand i got this assertion:
"""
TRAP: FailedAssertion("!(buf_state & (1U << 24))", File: "bufmgr.c", Line: 837)
LOG: server process (PID 18986) was terminated by signal 6: Aborted
"""
To reproduce:
1) run prepare_test.sql
2) then run the following pgbench command (sql scripts attached):
pgbench -c 24 -j 24 -T 600 -n -f inserts.sql@15 -f updates_1.sql@20 -f
updates_2.sql@20 -f deletes.sql@45 db_test

* sometimes when i have made the server crash the attempt to recovery
fails with this assertion:
"""
LOG: database system was not properly shut down; automatic recovery in progress
LOG: redo starts at 0/157F970
TRAP: FailedAssertion("!(!warm_update)", File: "heapam.c", Line: 8924)
LOG: startup process (PID 14031) was terminated by signal 6: Aborted
LOG: aborting startup due to startup process failure
"""
still cannot reproduce this one consistently but happens often enough

will continue playing with it...

--
Jaime Casanova www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Jaime Casanova wrote:

* The isolation test for partial_index fails (attached the regression.diffs)

Hmm, I had a very similar (if not identical) failure with indirect
indexes; in my case it was a bug in RelationGetIndexAttrBitmap() -- I
was missing to have HOT considerate the columns in index predicate, that
is, the second pull_varattnos() call.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Alvaro Herrera wrote:

Jaime Casanova wrote:

* The isolation test for partial_index fails (attached the regression.diffs)

Hmm, I had a very similar (if not identical) failure with indirect
indexes; in my case it was a bug in RelationGetIndexAttrBitmap() -- I
was missing to have HOT considerate the columns in index predicate, that
is, the second pull_varattnos() call.

Sorry, I meant:

Hmm, I had a very similar (if not identical) failure with indirect
indexes; in my case it was a bug in RelationGetIndexAttrBitmap() -- I
was missing to have HOT [take into account] the columns in index predicate, that
is, the second pull_varattnos() call.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Sat, Dec 24, 2016 at 1:18 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Alvaro Herrera wrote:

With your WARM and my indirect indexes, plus the additions for for-key
locks, plus identity columns, there is no longer a real expectation that
we can exit early from the function. In your patch, as well as mine,
there is a semblance of optimization that tries to avoid computing the
updated_attrs output bitmapset if the pointer is not passed in, but it's
effectively pointless because the only interesting use case is from
ExecUpdate() which always activates the feature. Can we just agree to
drop that?

Yes, I agree. As you noted below, the only case that may be affected is
simple_heap_update() which does a lot more and hence this function will be
least of the worries.

I think the only case that gets worse is the path that does
simple_heap_update, which is used for DDL. I would be very surprised if
a change there is noticeable, when compared to the rest of the stuff
that goes on for DDL commands.

Now, after saying that, I think that a table with a very large number of
columns is going to be affected by this. But we don't really need to
compute the output bits for every single column -- we only care about
those that are covered by some index. So we should pass an input
bitmapset comprising all such columns, and the output bitmapset only
considers those columns, and ignores columns not indexed. My patch for
indirect indexes already does something similar (though it passes a
bitmapset of columns indexed by indirect indexes only, so it needs a
tweak there.)

Yes, that looks like a good compromise. This would require us to compare
only those columns that any caller of the function might be interested in.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Dec 26, 2016 at 11:49 AM, Jaime Casanova <
jaime.casanova@2ndquadrant.com> wrote:

On 2 December 2016 at 07:36, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

I've updated the patches after fixing the issue. Multiple rounds of
regression passes for me without any issue. Please let me know if it

works

for you.

Hi Pavan,

Today i was playing with your patch and running some tests and found
some problems i wanted to report before i forget them ;)

Thanks Jaime for the tests and bug reports. I'm attaching an add-on patch
which fixes these issues for me. I'm deliberately not sending a fresh
revision because the changes are still minor.

* You need to add a prototype in src/backend/utils/adt/pgstatfuncs.c:
extern Datum pg_stat_get_tuples_warm_updated(PG_FUNCTION_ARGS);

Added.

* The isolation test for partial_index fails (attached the
regression.diffs)

Fixed. Looks like I forgot to include attributes from predicates and
expressions in the list of index attributes (as pointed by Alvaro)

* running a home-made test i have at hand i got this assertion:
"""
TRAP: FailedAssertion("!(buf_state & (1U << 24))", File: "bufmgr.c", Line:
837)
LOG: server process (PID 18986) was terminated by signal 6: Aborted
"""
To reproduce:
1) run prepare_test.sql
2) then run the following pgbench command (sql scripts attached):
pgbench -c 24 -j 24 -T 600 -n -f inserts.sql@15 -f updates_1.sql@20 -f
updates_2.sql@20 -f deletes.sql@45 db_test

Looks like the patch was failing to set the block number correctly in the
t_ctid field, leading to these strange failures. There was also couple of
instances where the t_ctid field was being accessed directly, instead of
the newly added macro. I think we need some better mechanism to ensure that
we don't miss out on such things. But I don't have a very good idea about
doing that right now.

* sometimes when i have made the server crash the attempt to recovery
fails with this assertion:
"""
LOG: database system was not properly shut down; automatic recovery in
progress
LOG: redo starts at 0/157F970
TRAP: FailedAssertion("!(!warm_update)", File: "heapam.c", Line: 8924)
LOG: startup process (PID 14031) was terminated by signal 6: Aborted
LOG: aborting startup due to startup process failure
"""
still cannot reproduce this one consistently but happens often enough

This could be a case of uninitialised variable in log_heap_update(). What
surprises me though that none of the compilers I tried so far could catch
that. In the following code snippet, if the condition evaluates to false
then "warm_update" may remain uninitialised, leading to wrong xlog entry,
which may later result in assertion failure during redo recovery.

7845
7846 if (HeapTupleIsHeapWarmTuple(newtup))
7847 warm_update = true;
7848

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Dec 27, 2016 at 6:51 PM, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

Thanks Jaime for the tests and bug reports. I'm attaching an add-on patch
which fixes these issues for me. I'm deliberately not sending a fresh
revision because the changes are still minor.

Per Alvaro's request in another thread, I've rebased these patches on his
patch to refactor HeapSatisfiesHOTandKeyUpdate(). I've also attached that
patch here for easy reference.

The fixes based on bug reports by Jaime are also included in this patch
set. Other than that there are not any significant changes. The patch still
disables WARM on system tables, something I would like to fix. But I've
been delaying that because it will require changes at several places since
indexes on system tables are managed separately. In addition to that, the
patch only works with btree and hash indexes. We must implement the recheck
method for other index types so as to support them.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Jan 3, 2017 at 9:43 AM, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

The patch still disables WARM on system tables, something I would like to
fix. But I've been delaying that because it will require changes at several
places since indexes on system tables are managed separately.

Here is another version which fixes a bug that I discovered while adding
support for system tables. The patch set now also includes a patch to
enable WARM on system tables. I'm attaching that as a separate patch
because while the changes to support WARM on system tables are many, almost
all of them are purely mechanical. We need to pass additional information
to CatalogUpdateIndexes()/CatalogIndexInsert(). We need to tell these
routines whether the update leading to them was a WARM update and which
columns were modified so that it can correctly avoid adding new index
tuples for indexes for which index keys haven't changed.

I wish I could find another way of passing this information instead of
making changes at so many places, but the only other way I could think of
was tracking that information as part of the HeapTuple itself, which
doesn't seem nice and may also require changes at many call sites where
tuples are constructed. One minor improvement could be that instead of two,
we could just pass "modified_attrs" and a NULL value may imply non-WARM
update. Other suggestions are welcome though.

I'm quite happy that all tests pass even after adding support for system
tables. One reason for testing support for system tables was to ensure some
more code paths get exercised. As before, I've included Alvaro's
refactoring patch too.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Reading through the track_root_lp patch now.

+		/*
+		 * For HOT (or WARM) updated tuples, we store the offset of the root
+		 * line pointer of this chain in the ip_posid field of the new tuple.
+		 * Usually this information will be available in the corresponding
+		 * field of the old tuple. But for aborted updates or pg_upgraded
+		 * databases, we might be seeing the old-style CTID chains and hence
+		 * the information must be obtained by hard way
+		 */
+		if (HeapTupleHeaderHasRootOffset(oldtup.t_data))
+			root_offnum = HeapTupleHeaderGetRootOffset(oldtup.t_data);
+		else
+			heap_get_root_tuple_one(page,
+					ItemPointerGetOffsetNumber(&(oldtup.t_self)),
+					&root_offnum);

Hmm. So the HasRootOffset tests the HEAP_LATEST_TUPLE bit, which is
reset temporarily during an update. So that case shouldn't occur often.

Oh, I just noticed that HeapTupleHeaderSetNextCtid also clears the flag.

@@ -4166,10 +4189,29 @@ l2:
HeapTupleClearHotUpdated(&oldtup);
HeapTupleClearHeapOnly(heaptup);
HeapTupleClearHeapOnly(newtup);
+ root_offnum = InvalidOffsetNumber;
}

-	RelationPutHeapTuple(relation, newbuf, heaptup, false);		/* insert new tuple */
+	/* insert new tuple */
+	RelationPutHeapTuple(relation, newbuf, heaptup, false, root_offnum);
+	HeapTupleHeaderSetHeapLatest(heaptup->t_data);
+	HeapTupleHeaderSetHeapLatest(newtup->t_data);

+	/*
+	 * Also update the in-memory copy with the root line pointer information
+	 */
+	if (OffsetNumberIsValid(root_offnum))
+	{
+		HeapTupleHeaderSetRootOffset(heaptup->t_data, root_offnum);
+		HeapTupleHeaderSetRootOffset(newtup->t_data, root_offnum);
+	}
+	else
+	{
+		HeapTupleHeaderSetRootOffset(heaptup->t_data,
+				ItemPointerGetOffsetNumber(&heaptup->t_self));
+		HeapTupleHeaderSetRootOffset(newtup->t_data,
+				ItemPointerGetOffsetNumber(&heaptup->t_self));
+	}

This is repetitive. I think after RelationPutHeapTuple it'd be better
to assign root_offnum = &heaptup->t_self, so that we can just call
SetRootOffset() on each tuple without the if().

+		HeapTupleHeaderSetHeapLatest((HeapTupleHeader) item);
+		if (OffsetNumberIsValid(root_offnum))
+			HeapTupleHeaderSetRootOffset((HeapTupleHeader) item,
+					root_offnum);
+		else
+			HeapTupleHeaderSetRootOffset((HeapTupleHeader) item,
+					offnum);

Just a matter of style, but this reads nicer IMO:

HeapTupleHeaderSetRootOffset((HeapTupleHeader) item,
OffsetNumberIsValid(root_offnum) ? root_offnum : offnum);

@@ -740,8 +742,9 @@ heap_page_prune_execute(Buffer buffer,
* holds a pin on the buffer. Once pin is released, a tuple might be pruned
* and reused by a completely unrelated tuple.
*/
-void
-heap_get_root_tuples(Page page, OffsetNumber *root_offsets)
+static void
+heap_get_root_tuples_internal(Page page, OffsetNumber target_offnum,
+		OffsetNumber *root_offsets)
{
OffsetNumber offnum,

I think this function deserves more/better/updated commentary.

@@ -439,7 +439,9 @@ rewrite_heap_tuple(RewriteState state,
* set the ctid of this tuple to point to the new location, and
* insert it right away.
*/
-			new_tuple->t_data->t_ctid = mapping->new_tid;
+			HeapTupleHeaderSetNextCtid(new_tuple->t_data,
+					ItemPointerGetBlockNumber(&mapping->new_tid),
+					ItemPointerGetOffsetNumber(&mapping->new_tid));

I think this would be nicer:
HeapTupleHeaderSetNextTid(new_tuple->t_data, &mapping->new_tid);
AFAICS all the callers are doing ItemPointerGetFoo for a TID, so this is
overly verbose for no reason. Also, the "c" in Ctid stands for
"current"; I think we can omit that.

@@ -525,7 +527,9 @@ rewrite_heap_tuple(RewriteState state,
new_tuple = unresolved->tuple;
free_new = true;
old_tid = unresolved->old_tid;
-				new_tuple->t_data->t_ctid = new_tid;
+				HeapTupleHeaderSetNextCtid(new_tuple->t_data,
+						ItemPointerGetBlockNumber(&new_tid),
+						ItemPointerGetOffsetNumber(&new_tid));

Did you forget to SetHeapLatest here, or ..? (If not, a comment is
warranted).

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 32bb3f9..466609c 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -2443,7 +2443,7 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
* As above, it should be safe to examine xmax and t_ctid without the
* buffer content lock, because they can't be changing.
*/
-		if (ItemPointerEquals(&tuple.t_self, &tuple.t_data->t_ctid))
+		if (HeapTupleHeaderIsHeapLatest(tuple.t_data, tuple.t_self))
{
/* deleted, so forget about it */
ReleaseBuffer(buffer);

This is the place where this patch would have an effect. To test this
bit I think we're going to need an ad-hoc stress-test harness.

+/*
+ * If HEAP_LATEST_TUPLE is set in the last tuple in the update chain. But for
+ * clusters which are upgraded from pre-10.0 release, we still check if c_tid
+ * is pointing to itself and declare such tuple as the latest tuple in the
+ * chain
+ */
+#define HeapTupleHeaderIsHeapLatest(tup, tid) \
+( \
+  ((tup)->t_infomask2 & HEAP_LATEST_TUPLE) || \
+  ((ItemPointerGetBlockNumber(&(tup)->t_ctid) == ItemPointerGetBlockNumber(&tid)) && \
+   (ItemPointerGetOffsetNumber(&(tup)->t_ctid) == ItemPointerGetOffsetNumber(&tid))) \
+)

Please add a "!= 0" to the first arm of the ||, so that we return a boolean.

+/*
+ * Get TID of next tuple in the update chain. Traditionally, we have stored
+ * self TID in the t_ctid field if the tuple is the last tuple in the chain. We
+ * try to preserve that behaviour by returning self-TID if HEAP_LATEST_TUPLE
+ * flag is set.
+ */
+#define HeapTupleHeaderGetNextCtid(tup, next_ctid, offnum) \
+do { \
+	if ((tup)->t_infomask2 & HEAP_LATEST_TUPLE) \
+	{ \
+		ItemPointerSet((next_ctid), ItemPointerGetBlockNumber(&(tup)->t_ctid), \
+				(offnum)); \
+	} \
+	else \
+	{ \
+		ItemPointerSet((next_ctid), ItemPointerGetBlockNumber(&(tup)->t_ctid), \
+				ItemPointerGetOffsetNumber(&(tup)->t_ctid)); \
+	} \
+} while (0)

This is a really odd macro, I think. Is any of the callers really
depending on the traditional behavior? If so, can we change them to
avoid that? (I think the "else" can be more easily written with
ItemPointerCopy). In any case, I think the documentation of the macro
leaves a bit to be desired -- I don't think we really care all that much
what we used to do, except perhaps as a secondary comment, but we do
care very much about what it actually does, which the current comment
doesn't really explain.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Hi Alvaro,

On Tue, Jan 17, 2017 at 8:41 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Reading through the track_root_lp patch now.

Thanks for the review.

+             /*
+              * For HOT (or WARM) updated tuples, we store the offset

of the root

+ * line pointer of this chain in the ip_posid field of the

new tuple.

+ * Usually this information will be available in the

corresponding

+ * field of the old tuple. But for aborted updates or

pg_upgraded

+ * databases, we might be seeing the old-style CTID chains

and hence

+              * the information must be obtained by hard way
+              */
+             if (HeapTupleHeaderHasRootOffset(oldtup.t_data))
+                     root_offnum = HeapTupleHeaderGetRootOffset(o

ldtup.t_data);

+             else
+                     heap_get_root_tuple_one(page,
+                                     ItemPointerGetOffsetNumber(&(

oldtup.t_self)),

+ &root_offnum);

Hmm. So the HasRootOffset tests the HEAP_LATEST_TUPLE bit, which is
reset temporarily during an update. So that case shouldn't occur often.

Right. The root offset is stored only in those tuples where
HEAP_LATEST_TUPLE is set. This flag should generally be set on the tuples
that are being updated, except for the case when the last update failed and
the flag was cleared. In other common case is going to be pg-upgraded
cluster where none of the existing tuples will have this flag set. So in
those cases, we must find the root line pointer hard way.

Oh, I just noticed that HeapTupleHeaderSetNextCtid also clears the flag.

Yes, but this should happen only during updates and unless the update
fails, the next-to-be-updated tuple should have the flag set.

@@ -4166,10 +4189,29 @@ l2:
HeapTupleClearHotUpdated(&oldtup);
HeapTupleClearHeapOnly(heaptup);
HeapTupleClearHeapOnly(newtup);
+             root_offnum = InvalidOffsetNumber;
}

- RelationPutHeapTuple(relation, newbuf, heaptup, false);

/* insert new tuple */

+     /* insert new tuple */
+     RelationPutHeapTuple(relation, newbuf, heaptup, false,

root_offnum);

+     HeapTupleHeaderSetHeapLatest(heaptup->t_data);
+     HeapTupleHeaderSetHeapLatest(newtup->t_data);

+     /*
+      * Also update the in-memory copy with the root line pointer

information

+      */
+     if (OffsetNumberIsValid(root_offnum))
+     {
+             HeapTupleHeaderSetRootOffset(heaptup->t_data,

root_offnum);

+             HeapTupleHeaderSetRootOffset(newtup->t_data, root_offnum);
+     }
+     else
+     {
+             HeapTupleHeaderSetRootOffset(heaptup->t_data,
+                             ItemPointerGetOffsetNumber(&h

eaptup->t_self));

+             HeapTupleHeaderSetRootOffset(newtup->t_data,
+                             ItemPointerGetOffsetNumber(&h

eaptup->t_self));

+ }

This is repetitive. I think after RelationPutHeapTuple it'd be better
to assign root_offnum = &heaptup->t_self, so that we can just call
SetRootOffset() on each tuple without the if().

Fixed. I actually ripped off HeapTupleHeaderSetRootOffset() completely and
pushed setting of root line pointer into the HeapTupleHeaderSetHeapLatest().
That seems much cleaner because the system expects to find root line
pointer whenever HEAP_LATEST_TUPLE flag is set. Hence it makes sense to set
them together.

+             HeapTupleHeaderSetHeapLatest((HeapTupleHeader) item);
+             if (OffsetNumberIsValid(root_offnum))
+                     HeapTupleHeaderSetRootOffset((HeapTupleHeader)

item,

+                                     root_offnum);
+             else
+                     HeapTupleHeaderSetRootOffset((HeapTupleHeader)

item,

+ offnum);

Just a matter of style, but this reads nicer IMO:

HeapTupleHeaderSetRootOffset((HeapTupleHeader) item,
OffsetNumberIsValid(root_offnum) ? root_offnum : offnum);

Understood. This code no longer exists in the new patch since
HeapTupleHeaderSetRootOffset is merged with HeapTupleHeaderSetHeapLatest.

@@ -740,8 +742,9 @@ heap_page_prune_execute(Buffer buffer,
* holds a pin on the buffer. Once pin is released, a tuple might be

pruned

* and reused by a completely unrelated tuple.
*/
-void
-heap_get_root_tuples(Page page, OffsetNumber *root_offsets)
+static void
+heap_get_root_tuples_internal(Page page, OffsetNumber target_offnum,
+             OffsetNumber *root_offsets)
{
OffsetNumber offnum,

I think this function deserves more/better/updated commentary.

Sure. I added more commentary. I also reworked the function so that the
caller can pass just one item array when it's interested in finding root
line pointer for just one item. Hopefully that will save a few bytes on the
stack.

@@ -439,7 +439,9 @@ rewrite_heap_tuple(RewriteState state,
* set the ctid of this tuple to point to the new

location, and

* insert it right away.
*/
-                     new_tuple->t_data->t_ctid = mapping->new_tid;
+                     HeapTupleHeaderSetNextCtid(new_tuple->t_data,
+                                     ItemPointerGetBlockNumber(&ma

pping->new_tid),

+ ItemPointerGetOffsetNumber(&m

apping->new_tid));

I think this would be nicer:
HeapTupleHeaderSetNextTid(new_tuple->t_data, &mapping->new_tid);
AFAICS all the callers are doing ItemPointerGetFoo for a TID, so this is
overly verbose for no reason. Also, the "c" in Ctid stands for
"current"; I think we can omit that.

Yes, fixed. I realised that all callers where anyways calling the macro
with the block/offset of the same TID. So it makes sense to just pass TID
to the macro.

@@ -525,7 +527,9 @@ rewrite_heap_tuple(RewriteState state,
new_tuple = unresolved->tuple;
free_new = true;
old_tid = unresolved->old_tid;
-                             new_tuple->t_data->t_ctid = new_tid;
+                             HeapTupleHeaderSetNextCtid(ne

w_tuple->t_data,

+

ItemPointerGetBlockNumber(&new_tid),

+

ItemPointerGetOffsetNumber(&new_tid));

Did you forget to SetHeapLatest here, or ..? (If not, a comment is
warranted).

Umm probably not. The way I see it, new_tuple is not actually the new tuple
when this is called, but it's changed to the unresolved tuple (see the
start of the hunk). So what we're doing is setting next CTID in the
previous tuple in the chain. SetHeapLatest is called on the new tuple
inside raw_heap_insert(). I did not add any more comments, but please let
me know if you think it's still confusing or if I'm missing something.

diff --git a/src/backend/executor/execMain.c

b/src/backend/executor/execMain.c

index 32bb3f9..466609c 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -2443,7 +2443,7 @@ EvalPlanQualFetch(EState *estate, Relation

relation, int lockmode,

* As above, it should be safe to examine xmax and t_ctid

without the

* buffer content lock, because they can't be changing.
*/
- if (ItemPointerEquals(&tuple.t_self,

&tuple.t_data->t_ctid))

+ if (HeapTupleHeaderIsHeapLatest(tuple.t_data,

tuple.t_self))

{
/* deleted, so forget about it */
ReleaseBuffer(buffer);

This is the place where this patch would have an effect. To test this
bit I think we're going to need an ad-hoc stress-test harness.

Sure. I did some pgbench tests and ran consistency checks during and at the
end of tests. I chose a small scale factor and many clients so that same
tuple is often concurrently updated. That should exercise the new
chain-following code reguorsly. But I'll do more of those on a bigger box.
Do you have other suggestions for ad-hoc tests?

+/*
+ * If HEAP_LATEST_TUPLE is set in the last tuple in the update chain.

But for

+ * clusters which are upgraded from pre-10.0 release, we still check if

c_tid

+ * is pointing to itself and declare such tuple as the latest tuple in

the

+ * chain
+ */
+#define HeapTupleHeaderIsHeapLatest(tup, tid) \
+( \
+  ((tup)->t_infomask2 & HEAP_LATEST_TUPLE) || \
+  ((ItemPointerGetBlockNumber(&(tup)->t_ctid) ==

ItemPointerGetBlockNumber(&tid)) && \

+ (ItemPointerGetOffsetNumber(&(tup)->t_ctid) ==

ItemPointerGetOffsetNumber(&tid))) \

+)

Please add a "!= 0" to the first arm of the ||, so that we return a
boolean.

Done. Also rebased with new master where similar changes have been done.

+/*
+ * Get TID of next tuple in the update chain. Traditionally, we have

stored

+ * self TID in the t_ctid field if the tuple is the last tuple in the

chain. We

+ * try to preserve that behaviour by returning self-TID if

HEAP_LATEST_TUPLE

+ * flag is set.
+ */
+#define HeapTupleHeaderGetNextCtid(tup, next_ctid, offnum) \
+do { \
+     if ((tup)->t_infomask2 & HEAP_LATEST_TUPLE) \
+     { \
+             ItemPointerSet((next_ctid), ItemPointerGetBlockNumber(&(tup)->t_ctid),

\

+                             (offnum)); \
+     } \
+     else \
+     { \
+             ItemPointerSet((next_ctid), ItemPointerGetBlockNumber(&(tup)->t_ctid),

\

+ ItemPointerGetOffsetNumber(&(tup)->t_ctid));

\

+ } \
+} while (0)

This is a really odd macro, I think. Is any of the callers really
depending on the traditional behavior? If so, can we change them to
avoid that? (I think the "else" can be more easily written with
ItemPointerCopy). In any case, I think the documentation of the macro
leaves a bit to be desired -- I don't think we really care all that much
what we used to do, except perhaps as a secondary comment, but we do
care very much about what it actually does, which the current comment
doesn't really explain.

I reworked this quite a bit and I believe the new code does what you
suggested. The HeapTupleHeaderGetNextTid macro is now much simpler (it
just copies the TID) and we leave it to the caller to ensure they don't
call this on a tuple which is already at the end of the chain (i.e has
HEAP_LATEST_TUPLE set, but we don't look for old-style end-of-the-chain
markers). The callers can choose to return the same TID back if their
callers rely on that behaviour. But inside this macro, we now assert that
HEAP_LATEST_TUPLE is not set.

One thing that worried me is if there exists a path which sets the
t_infomask (and hence HEAP_LATEST_TUPLE) during redo recovery and if we
will fail to set the root line pointer correctly along with that. But
AFAICS the interesting cases of insert, multi-insert and update are being
handled ok. The only other places where I saw t_infomask being copied as-is
from the WAL record is DecodeXLogTuple() and DecodeMultiInsert(), but those
should not cause any problem AFAICS.

Revised patch is attached. All regression tests, isolation tests and
pgbench test with -c40 -j10 pass on my laptop.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Jan 19, 2017 at 6:35 PM, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

Revised patch is attached.

I've now also rebased the main WARM patch against the current master
(3eaf03b5d331b7a06d79 to be precise). I'm attaching Alvaro's patch to get
interesting attributes (prefixed with 0000 since the other two patches are
based on that). The changes to support system tables are now merged with
the main patch. I could separate them if it helps in review.

I am also including a stress test workload that I am currently running to
test WARM's correctness since Robert raised a valid concern about that. The
idea is to include a few more columns in the pgbench_accounts table and
have a few more indexes. The additional columns with indexes kind of share
a relationship with the "aid" column. But instead of a fixed value, values
for these columns can vary within a fixed, non-overlapping range. For
example, for aid = 1, aid1's original value will be 10 and it can vary
between 8 to 12. Similarly, aid2's original value will be 20 and it can
vary between 16 to 24. This setup allows us to update these additional
columns (thus force WARM), but still ensure that we can do some sanity
checks on the results.

The test contains a bunch of UPDATE, FOR UPDATE, FOR SHARE transactions.
Some of these transactions commit and some rollback. The checks are
in-place to ensure that we always find exactly one tuple irrespective of
which column we use to fetch the row. Of course, when the aid[1-4] columns
are used to fetch tuples, we need to scan with a range instead of an
equality. Then we do a bunch of operations like CREATE INDEX, DROP INDEX,
CIC, run long transactions, VACUUM FULL etc while the tests are running and
ensure that the sanity checks always pass. We could do a few other things
like, may be marking these indexes as UNIQUE or keeping a long transaction
open while doing updates and other operations. I'll add some of those to
the test, but suggestions are welcome.

I do see a problem with CREATE INDEX CONCURRENTLY with these tests, though
everything else has run ok so far (I am yet to do very long running tests.
Probably just a few hours tests today).

I'm trying to understand why CIC fails to build a consistent index. I think
I've some clue now why it could be happening. With HOT, we don't need to
worry about broken chains since at the very beginning we add the index
tuple and all subsequent updates will honour the new index while deciding
on HOT updates i.e. we won't create any new broken HOT chains once we start
building the index. Later during validation phase, we only need to insert
tuples that are not already in the index. But with WARM, I think the check
needs to be more elaborate. So even if the TID (we always look at its root
line pointer etc) exists in the index, we will need to ensure that the
index key matches the heap tuple we are dealing with. That looks a bit
tricky. May be we can lookup the index using key from the current heap
tuple and then see if we get a tuple with the same TID back. Of course, we
need to do this only if the tuple is a WARM tuple. The other option is that
we collect not only TIDs but also keys while scanning the index. That might
increase the size of the state information for wildly wide indexes. Or may
be just turn WARM off if there exists a build-in-progress index.

Suggestions/reviews/tests welcome.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Reading 0001_track_root_lp_v9.patch again:

+/*
+ * We use the same HEAP_LATEST_TUPLE flag to check if the tuple's t_ctid field
+ * contains the root line pointer. We can't use the same
+ * HeapTupleHeaderIsHeapLatest macro because that also checks for TID-equality
+ * to decide whether a tuple is at the of the chain
+ */
+#define HeapTupleHeaderHasRootOffset(tup) \
+( \
+	((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0 \
+)

+#define HeapTupleHeaderGetRootOffset(tup) \
+( \
+	AssertMacro(((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0), \
+	ItemPointerGetOffsetNumber(&(tup)->t_ctid) \
+)

Interesting stuff; it took me a bit to see why these macros are this
way. I propose the following wording which I think is clearer:

Return whether the tuple has a cached root offset. We don't use
HeapTupleHeaderIsHeapLatest because that one also considers the slow
case of scanning the whole block.

Please flag the macros that have multiple evaluation hazards -- there
are a few of them.

+/*
+ * If HEAP_LATEST_TUPLE is set in the last tuple in the update chain. But for
+ * clusters which are upgraded from pre-10.0 release, we still check if c_tid
+ * is pointing to itself and declare such tuple as the latest tuple in the
+ * chain
+ */
+#define HeapTupleHeaderIsHeapLatest(tup, tid) \
+( \
+  (((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0) || \
+  ((ItemPointerGetBlockNumber(&(tup)->t_ctid) == ItemPointerGetBlockNumber(tid)) && \
+   (ItemPointerGetOffsetNumber(&(tup)->t_ctid) == ItemPointerGetOffsetNumber(tid))) \
+)

I suggest rewording this comment as:
Starting from PostgreSQL 10, the latest tuple in an update chain has
HEAP_LATEST_TUPLE set; but tuples upgraded from earlier versions do
not. For those, we determine whether a tuple is latest by testing
that its t_ctid points to itself.
(as discussed, there is no "10.0 release"; it's called the "10 release"
only, no ".0". Feel free to use "v10" or "pg10").

+/*
+ * Get TID of next tuple in the update chain. Caller should have checked that
+ * we are not already at the end of the chain because in that case t_ctid may
+ * actually store the root line pointer of the HOT chain whose member this
+ * tuple is.
+ */
+#define HeapTupleHeaderGetNextTid(tup, next_ctid) \
+do { \
+	AssertMacro(!((tup)->t_infomask2 & HEAP_LATEST_TUPLE)); \
+	ItemPointerCopy(&(tup)->t_ctid, (next_ctid)); \
+} while (0)

Actually, I think this macro could just return the TID so that it can be
used as struct assignment, just like ItemPointerCopy does internally --
callers can do
ctid = HeapTupleHeaderGetNextTid(tup);

or more precisely, this pattern

+		if (!HeapTupleHeaderIsHeapLatest(tp.t_data, &tp.t_self))
+			HeapTupleHeaderGetNextTid(tp.t_data, &hufd->ctid);
+		else
+			ItemPointerCopy(&tp.t_self, &hufd->ctid);

becomes
hufd->ctid = HeapTupleHeaderIsHeapLatest(foo) ?
HeapTupleHeaderGetNextTid(foo) : &tp->t_self;
or something like that. I further wonder if it'd make sense to hide
this into yet another macro.

The API of RelationPutHeapTuple appears a bit contorted, where
root_offnum is both input and output. I think it's cleaner to have the
argument be the input, and have the output offset be the return value --
please check whether that simplifies things; for example I think this:

+			root_offnum = InvalidOffsetNumber;
+			RelationPutHeapTuple(relation, buffer, heaptup, false,
+					&root_offnum);

becomes

root_offnum = RelationPutHeapTuple(relation, buffer, heaptup, false,
InvalidOffsetNumber);

Please remove the words "must have" in this comment:

+	/*
+	 * Also mark both copies as latest and set the root offset information. If
+	 * we're doing a HOT/WARM update, then we just copy the information from
+	 * old tuple, if available or computed above. For regular updates,
+	 * RelationPutHeapTuple must have returned us the actual offset number
+	 * where the new version was inserted and we store the same value since the
+	 * update resulted in a new HOT-chain
+	 */

Many comments lack finishing periods in complete sentences, which looks
odd. Please fix.

I have not looked at the other patch yet.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Looking at your 0002 patch now. It no longer applies, but the conflicts
are trivial to fix. Please rebase and resubmit.

I think the way WARM works has been pretty well hammered by now, other
than the CREATE INDEX CONCURRENTLY issues, so I'm looking at the code
from a maintainability point of view only.

I think we should have some test harness for WARM as part of the source
repository. A test that runs for an hour hammering the machine to
highest possible load cannot be run in "make check", of course, but we
could have some specific Make target to run it manually. We don't have
this for any other feature, but this looks like a decent place to start.
Maybe we should even do it before going any further. The test code you
submitted looks OK to test the feature, but I'm not in love with it
enough to add it to the repo. Maybe I will spend some time trying to
convert it to Perl using PostgresNode.

I think having the "recheck" index methods create an ExecutorState looks
out of place. How difficult is it to pass the estate from the calling
code?

IMO heap_get_root_tuple_one should be called just heap_get_root_tuple().
That function and its plural sibling heap_get_root_tuples() should
indicate in their own comments what the expectations are regarding the
root_offsets output argument, rather than deferring to the comments in
the "internal" function, since they differ on that point; for the rest
of the invariants I think it makes sense to say "Also see the comment
for heap_get_root_tuples_internal". I wonder if heap_get_root_tuple
should just return the ctid instead of assigning the value to a
passed-in pointer, i.e.
OffsetNumber
heap_get_root_tuple(Page page, OffsetNumber target_offnum)
{
OffsetNumber off;
heap_get_root_tuples_internal(page, target_offnum, &off);
return off;
}

The simple_heap_update + CatalogUpdateIndexes pattern is getting
obnoxious. How about creating something like catalog_heap_update which
does both things at once, and stop bothering each callsite with the WARM
stuff? In fact, given that CatalogUpdateIndexes is used in other
places, maybe we should leave its API alone and create another function,
so that we don't have to change the many places that only do
simple_heap_insert. (Places like OperatorCreate which do either insert
or update could just move the index update call into each branch.)

I'm not real sure about the interface between index AM and executor,
namely IndexScanDesc->xs_tuple_recheck. For example, this pattern:
if (!scan->xs_recheck)
scan->xs_tuple_recheck = false;
else
scan->xs_tuple_recheck = true;
can become simply
scan->xs_tuple_recheck = scan->xs_recheck;
which looks odd. I can't pinpoint exactly what's the problem, though.
I'll continue looking at this one.

I wonder if heap_hot_search_buffer() and heap_hot_search() should return
a tri-valued enum instead of boolean; that idea looks reasonable in
theory but callers have to do more work afterwards, so maybe not.

I think heap_hot_search() sometimes leaving the buffer pinned is
confusing. Really, the whole idea of having heap_hot_search have a
buffer output argument is an important API change that should be better
thought. Maybe it'd be better to return the buffer pinned always, and
the caller is always in charge of unpinning if not InvalidBuffer. Or
perhaps we need a completely new function, given how different it is to
the original? If you tried to document in the comment above
heap_hot_search how it works, you'd find that it's difficult to
describe, which'd be an indicator that it's not well considered.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Alvaro Herrera wrote:

I wonder if heap_hot_search_buffer() and heap_hot_search() should return
a tri-valued enum instead of boolean; that idea looks reasonable in
theory but callers have to do more work afterwards, so maybe not.

I think heap_hot_search() sometimes leaving the buffer pinned is
confusing. Really, the whole idea of having heap_hot_search have a
buffer output argument is an important API change that should be better
thought. Maybe it'd be better to return the buffer pinned always, and
the caller is always in charge of unpinning if not InvalidBuffer. Or
perhaps we need a completely new function, given how different it is to
the original? If you tried to document in the comment above
heap_hot_search how it works, you'd find that it's difficult to
describe, which'd be an indicator that it's not well considered.

Even before your patch, heap_hot_search claims to have the same API as
heap_hot_search_buffer "except that caller does not provide the buffer."
But this is a lie and has been since 9.2 (more precisely, since commit
4da99ea4231e). I think WARM makes things even worse and we should fix
that. Not yet sure which direction to fix it ...

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, Jan 25, 2017 at 4:08 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

I think the way WARM works has been pretty well hammered by now, other
than the CREATE INDEX CONCURRENTLY issues, so I'm looking at the code
from a maintainability point of view only.

Which senior hackers have previously reviewed it in detail?

Where would I go to get a good overview of the overall theory of operation?

--
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

Robert Haas wrote:

On Wed, Jan 25, 2017 at 4:08 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

I think the way WARM works has been pretty well hammered by now, other
than the CREATE INDEX CONCURRENTLY issues, so I'm looking at the code
from a maintainability point of view only.

Which senior hackers have previously reviewed it in detail?

The previous thread,
/messages/by-id/CABOikdMop5Rb_RnS2xFdAXMZGSqcJ-P-BY2ruMd+buUkJ4iDPw@mail.gmail.com
contains some discussion of it, which uncovered bugs in the initial idea
and gave rise to the current design.

Where would I go to get a good overview of the overall theory of operation?

The added README file does a pretty good job, I thought.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, Jan 25, 2017 at 10:06 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Reading 0001_track_root_lp_v9.patch again:

Thanks for the review.

+/*
+ * We use the same HEAP_LATEST_TUPLE flag to check if the tuple's

t_ctid field

+ * contains the root line pointer. We can't use the same
+ * HeapTupleHeaderIsHeapLatest macro because that also checks for

TID-equality

+ * to decide whether a tuple is at the of the chain
+ */
+#define HeapTupleHeaderHasRootOffset(tup) \
+( \
+     ((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0 \
+)

+#define HeapTupleHeaderGetRootOffset(tup) \
+( \
+     AssertMacro(((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0), \
+     ItemPointerGetOffsetNumber(&(tup)->t_ctid) \
+)

Interesting stuff; it took me a bit to see why these macros are this
way. I propose the following wording which I think is clearer:

Return whether the tuple has a cached root offset. We don't use
HeapTupleHeaderIsHeapLatest because that one also considers the slow
case of scanning the whole block.

Umm, not scanning the whole block, but HeapTupleHeaderIsHeapLatest compares
t_ctid with the passed in TID and returns true if those matches. To know if
root lp is cached, we only rely on the HEAP_LATEST_TUPLE flag. Though if
the flag is set, then it implies latest tuple too.

Please flag the macros that have multiple evaluation hazards -- there
are a few of them.

Can you please tell me an example? I must be missing something.

+/*
+ * Get TID of next tuple in the update chain. Caller should have

checked that

+ * we are not already at the end of the chain because in that case

t_ctid may

+ * actually store the root line pointer of the HOT chain whose member

this

+ * tuple is.
+ */
+#define HeapTupleHeaderGetNextTid(tup, next_ctid) \
+do { \
+     AssertMacro(!((tup)->t_infomask2 & HEAP_LATEST_TUPLE)); \
+     ItemPointerCopy(&(tup)->t_ctid, (next_ctid)); \
+} while (0)

Actually, I think this macro could just return the TID so that it can be
used as struct assignment, just like ItemPointerCopy does internally --
callers can do
ctid = HeapTupleHeaderGetNextTid(tup);

Yes, makes sense. Will fix.

The API of RelationPutHeapTuple appears a bit contorted, where
root_offnum is both input and output. I think it's cleaner to have the
argument be the input, and have the output offset be the return value --
please check whether that simplifies things; for example I think this:

+                     root_offnum = InvalidOffsetNumber;
+                     RelationPutHeapTuple(relation, buffer, heaptup,

false,

+ &root_offnum);

becomes

root_offnum = RelationPutHeapTuple(relation, buffer, heaptup,
false,
InvalidOffsetNumber);

Make sense. Will fix.

Many comments lack finishing periods in complete sentences, which looks
odd. Please fix.

Sorry, not sure where I picked that style from. I see that the existing
code has both styles, though I will add finishing periods because I like
that way too.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Jan 26, 2017 at 2:38 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Looking at your 0002 patch now. It no longer applies, but the conflicts
are trivial to fix. Please rebase and resubmit.

Thanks.

Maybe I will spend some time trying to
convert it to Perl using PostgresNode.

Agree. I put together a test harness to hammer the WARM code as much as we
can. This harness has already discovered some bugs, especially around index
creation part. It also discovered one outstanding bug in master, so it's
been useful. But I agree to rewrite it using perl.

I think having the "recheck" index methods create an ExecutorState looks
out of place. How difficult is it to pass the estate from the calling
code?

I couldn't find an easy way given the place where recheck is required. Can
you suggest something?

IMO heap_get_root_tuple_one should be called just heap_get_root_tuple().
That function and its plural sibling heap_get_root_tuples() should
indicate in their own comments what the expectations are regarding the
root_offsets output argument, rather than deferring to the comments in
the "internal" function, since they differ on that point; for the rest
of the invariants I think it makes sense to say "Also see the comment
for heap_get_root_tuples_internal". I wonder if heap_get_root_tuple
should just return the ctid instead of assigning the value to a
passed-in pointer, i.e.
OffsetNumber
heap_get_root_tuple(Page page, OffsetNumber target_offnum)
{
OffsetNumber off;
heap_get_root_tuples_internal(page, target_offnum, &off);
return off;
}

Yes, all of that makes sense. Will fix.

The simple_heap_update + CatalogUpdateIndexes pattern is getting
obnoxious. How about creating something like catalog_heap_update which
does both things at once, and stop bothering each callsite with the WARM
stuff? In fact, given that CatalogUpdateIndexes is used in other
places, maybe we should leave its API alone and create another function,
so that we don't have to change the many places that only do
simple_heap_insert. (Places like OperatorCreate which do either insert
or update could just move the index update call into each branch.)

What I ended up doing is I added two new APIs.
- CatalogUpdateHeapAndIndex
- CatalogInsertHeapAndIndex

I could replace almost all occurrences of simple_heap_update +
CatalogUpdateIndexes with the first API and simple_heap_insert +
CatalogUpdateIndexes with the second API. This looks like a good
improvement to me anyways since there are about 180 places where these
functions are called almost in the same pattern. May be it will also avoid
a bug when someone forgets to update the index after inserting/updating
heap.

.
I wonder if heap_hot_search_buffer() and heap_hot_search() should return
a tri-valued enum instead of boolean; that idea looks reasonable in
theory but callers have to do more work afterwards, so maybe not.

Ok. I'll try to rearrange it a bit. May be we just have one API after all?
There are only a very few callers of these APIs.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavan Deolasee wrote:

On Wed, Jan 25, 2017 at 10:06 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

+( \
+     ((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0 \
+)

+#define HeapTupleHeaderGetRootOffset(tup) \
+( \
+     AssertMacro(((tup)->t_infomask2 & HEAP_LATEST_TUPLE) != 0), \
+     ItemPointerGetOffsetNumber(&(tup)->t_ctid) \
+)

Interesting stuff; it took me a bit to see why these macros are this
way. I propose the following wording which I think is clearer:

Return whether the tuple has a cached root offset. We don't use
HeapTupleHeaderIsHeapLatest because that one also considers the slow
case of scanning the whole block.

Umm, not scanning the whole block, but HeapTupleHeaderIsHeapLatest compares
t_ctid with the passed in TID and returns true if those matches. To know if
root lp is cached, we only rely on the HEAP_LATEST_TUPLE flag. Though if
the flag is set, then it implies latest tuple too.

Well, I'm just trying to fix the problem that when I saw that macro, I
thought "why is this checking the bitmask directly instead of using the
existing IsHeapLatest macro?" when I saw the code. It turned out that
IsHeapLatest is not just simply comparing the bitmask, but it also does
more expensive processing which is unwanted in this case. I think the
comment to this macro should explain why the other macro cannot be used.

Please flag the macros that have multiple evaluation hazards -- there
are a few of them.

Can you please tell me an example? I must be missing something.

Any macro that uses an argument more than once is subject to multiple
evaluations of that argument; for example, if you pass a function call to
the macro as one of the parameters, the function is called multiple
times. In many cases this is not a problem because the argument is
always a constant, but sometimes it does become a problem.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Thu, Jan 26, 2017 at 2:38 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Looking at your 0002 patch now. It no longer applies, but the conflicts
are trivial to fix. Please rebase and resubmit.

Please see rebased and updated patches attached.

I think having the "recheck" index methods create an ExecutorState looks
out of place. How difficult is it to pass the estate from the calling
code?

I couldn't find a good way to pass estate from the calling code. It would
require changes to many other APIs. I saw all other callers who need to
form index keys do that too. But please suggest if there are better ways.

OffsetNumber
heap_get_root_tuple(Page page, OffsetNumber target_offnum)
{
OffsetNumber off;
heap_get_root_tuples_internal(page, target_offnum, &off);
return off;
}

Ok. Changed this way. Definitely looks better.

The simple_heap_update + CatalogUpdateIndexes pattern is getting
obnoxious. How about creating something like catalog_heap_update which
does both things at once, and stop bothering each callsite with the WARM
stuff?

What I realised that there are really 2 patterns:
1. simple_heap_insert, CatalogUpdateIndexes
2. simple_heap_update, CatalogUpdateIndexes

There are only couple of places where we already have indexes open or have
more than one tuple to update, so we call CatalogIndexInsert directly. What
I ended up doing in the attached patch is add two new APIs which combines
the two steps of each of these patterns. It seems much cleaner to me and
also less buggy for future users. I hope I am not missing a reason not to
do combine these steps.

I'm not real sure about the interface between index AM and executor,
namely IndexScanDesc->xs_tuple_recheck. For example, this pattern:
if (!scan->xs_recheck)
scan->xs_tuple_recheck = false;
else
scan->xs_tuple_recheck = true;
can become simply
scan->xs_tuple_recheck = scan->xs_recheck;

Fixed.

which looks odd. I can't pinpoint exactly what's the problem, though.
I'll continue looking at this one.

What we do is if the index scan is marked to do recheck, we do it for each
tuple anyways. Otherwise recheck is required only if a tuple comes from a
WARM chain.

I wonder if heap_hot_search_buffer() and heap_hot_search() should return
a tri-valued enum instead of boolean; that idea looks reasonable in
theory but callers have to do more work afterwards, so maybe not.

I did not do anything with this yet. But I agree with you that we need to
make it better/simpler. Will continue to work on that.

I've addressed other review comments on the 0001 patch, except this one.

+/*
+ * Get TID of next tuple in the update chain. Caller should have checked

that

+ * we are not already at the end of the chain because in that case

t_ctid may

+ * actually store the root line pointer of the HOT chain whose member

this

+ * tuple is.
+ */
+#define HeapTupleHeaderGetNextTid(tup, next_ctid) \
+do { \
+     AssertMacro(!((tup)->t_infomask2 & HEAP_LATEST_TUPLE)); \
+     ItemPointerCopy(&(tup)->t_ctid, (next_ctid)); \
+} while (0)

Actually, I think this macro could just return the TID so that it can be
used as struct assignment, just like ItemPointerCopy does internally --
callers can do
ctid = HeapTupleHeaderGetNextTid(tup);

While I agree with your proposal, I wonder why we have ItemPointerCopy() in
the first place because we freely copy TIDs as struct assignment. Is there
a reason for that? And if there is, does it impact this specific case?

Other than the review comments, there were couple of bugs that I discovered
while running the stress test notably around visibility map handling. The
patch has those fixes. I also ripped out the kludge to record WARM-ness in
the line pointer because that is no longer needed after I reworked the code
a few versions back.

The other critical bug I found, which unfortunately exists in the master
too, is the index corruption during CIC. The patch includes the same fix
that I've proposed on the other thread. With these changes, WARM stress is
running fine for last 24 hours on a decently powerful box. Multiple
CREATE/DROP INDEX cycles and updates via different indexed columns, with a
mix of FOR SHARE/UPDATE and rollbacks did not produce any consistency
issues. A side note: while performance measurement wasn't a goal of stress
tests, WARM has done about 67% more transaction than master in 24 hour
period (95M in master vs 156M in WARM to be precise on a 30GB table
including indexes). I believe the numbers would be far better had the test
not dropping and recreating the indexes, thus effectively cleaning up all
index bloats. Also the table is small enough to fit in the shared buffers.
I'll rerun these tests with much larger scale factor and without dropping
indexes.

Of course, make check-world, including all TAP tests, passes too.

The CREATE INDEX CONCURRENTLY now works. The way we handle this is by
ensuring that no broken WARM chains are created while the initial index
build is happening. We check the list of attributes of indexes currently
in-progress (i.e. not ready for inserts) and if any of these attributes are
being modified, we don't do a WARM update. This is enough to address CIC
issue and all other mechanisms remain same as HOT. I've updated README to
include CIC algorithm.

There is one issue that bothers me. The current implementation lacks
ability to convert WARM chains into HOT chains. The README.WARM has some
proposal to do that. But it requires additional free bit in tuple header
(which we don't have) and of course, it needs to be vetted and implemented.
If the heap ends up with many WARM tuples, then index-only-scans will
become ineffective because index-only-scan can not skip a heap page, if it
contains a WARM tuple. Alternate ideas/suggestions and review of the design
are welcome!

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavan Deolasee wrote:

On Thu, Jan 26, 2017 at 2:38 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

The simple_heap_update + CatalogUpdateIndexes pattern is getting
obnoxious. How about creating something like catalog_heap_update which
does both things at once, and stop bothering each callsite with the WARM
stuff?

What I realised that there are really 2 patterns:
1. simple_heap_insert, CatalogUpdateIndexes
2. simple_heap_update, CatalogUpdateIndexes

There are only couple of places where we already have indexes open or have
more than one tuple to update, so we call CatalogIndexInsert directly. What
I ended up doing in the attached patch is add two new APIs which combines
the two steps of each of these patterns. It seems much cleaner to me and
also less buggy for future users. I hope I am not missing a reason not to
do combine these steps.

CatalogUpdateIndexes was just added as a convenience function on top of
a very common pattern. If we now have a reason to create a second one
because there are now two very common patterns, it seems reasonable to
have two functions. I think I would commit the refactoring to create
these functions ahead of the larger WARM patch, since I think it'd be
bulky and largely mechanical. (I'm going from this description; didn't
read your actual code.)

+#define HeapTupleHeaderGetNextTid(tup, next_ctid) \
+do { \
+     AssertMacro(!((tup)->t_infomask2 & HEAP_LATEST_TUPLE)); \
+     ItemPointerCopy(&(tup)->t_ctid, (next_ctid)); \
+} while (0)

Actually, I think this macro could just return the TID so that it can be
used as struct assignment, just like ItemPointerCopy does internally --
callers can do
ctid = HeapTupleHeaderGetNextTid(tup);

While I agree with your proposal, I wonder why we have ItemPointerCopy() in
the first place because we freely copy TIDs as struct assignment. Is there
a reason for that? And if there is, does it impact this specific case?

I dunno. This macro is present in our very first commit d31084e9d1118b.
Maybe it's an artifact from the Lisp to C conversion. Even then, we had
some cases of iptrs being copied by struct assignment, so it's not like
it didn't work. Perhaps somebody envisioned that the internal details
could change, but that hasn't happened in two decades so why should we
worry about it now? If somebody needs it later, it can be changed then.

There is one issue that bothers me. The current implementation lacks
ability to convert WARM chains into HOT chains. The README.WARM has some
proposal to do that. But it requires additional free bit in tuple header
(which we don't have) and of course, it needs to be vetted and implemented.
If the heap ends up with many WARM tuples, then index-only-scans will
become ineffective because index-only-scan can not skip a heap page, if it
contains a WARM tuple. Alternate ideas/suggestions and review of the design
are welcome!

t_infomask2 contains one last unused bit, and we could reuse vacuum
full's bits (HEAP_MOVED_OUT, HEAP_MOVED_IN), but that will need some
thinking ahead. Maybe now's the time to start versioning relations so
that we can ensure clusters upgraded to pg10 do not contain any of those
bits in any tuple headers.

I don't have any ideas regarding the estate passed to recheck yet --
haven't looked at the callsites in detail. I'll give this another look
later.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Tue, Jan 31, 2017 at 7:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Pavan Deolasee wrote:

On Thu, Jan 26, 2017 at 2:38 AM, Alvaro Herrera <

alvherre@2ndquadrant.com>

wrote:

The simple_heap_update + CatalogUpdateIndexes pattern is getting
obnoxious. How about creating something like catalog_heap_update which
does both things at once, and stop bothering each callsite with the

WARM

stuff?

What I realised that there are really 2 patterns:
1. simple_heap_insert, CatalogUpdateIndexes
2. simple_heap_update, CatalogUpdateIndexes

There are only couple of places where we already have indexes open or

have

more than one tuple to update, so we call CatalogIndexInsert directly.

What

I ended up doing in the attached patch is add two new APIs which combines
the two steps of each of these patterns. It seems much cleaner to me and
also less buggy for future users. I hope I am not missing a reason not to
do combine these steps.

CatalogUpdateIndexes was just added as a convenience function on top of
a very common pattern. If we now have a reason to create a second one
because there are now two very common patterns, it seems reasonable to
have two functions. I think I would commit the refactoring to create
these functions ahead of the larger WARM patch, since I think it'd be
bulky and largely mechanical. (I'm going from this description; didn't
read your actual code.)

Sounds good. Should I submit that as a separate patch on current master?

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavan Deolasee wrote:

On Tue, Jan 31, 2017 at 7:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

CatalogUpdateIndexes was just added as a convenience function on top of
a very common pattern. If we now have a reason to create a second one
because there are now two very common patterns, it seems reasonable to
have two functions. I think I would commit the refactoring to create
these functions ahead of the larger WARM patch, since I think it'd be
bulky and largely mechanical. (I'm going from this description; didn't
read your actual code.)

Sounds good. Should I submit that as a separate patch on current master?

Yes, please.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Tue, Jan 31, 2017 at 7:37 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Pavan Deolasee wrote:

Sounds good. Should I submit that as a separate patch on current master?

Yes, please.

Attached.

Two new APIs added.

- CatalogInsertHeapAndIndex which does a simple_heap_insert followed by
catalog updates
- CatalogUpdateHeapAndIndex which does a simple_heap_update followed by
catalog updates

There are only a handful callers remain for simple_heap_insert/update after
this patch. They are typically working with already opened indexes and
hence I left them unchanged.

make check-world passes with the patch.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavan Deolasee wrote:

Two new APIs added.

- CatalogInsertHeapAndIndex which does a simple_heap_insert followed by
catalog updates
- CatalogUpdateHeapAndIndex which does a simple_heap_update followed by
catalog updates

There are only a handful callers remain for simple_heap_insert/update after
this patch. They are typically working with already opened indexes and
hence I left them unchanged.

Hmm, I was thinking we would get rid of CatalogUpdateIndexes altogether.
Two of the callers are in the new routines (which I propose to rename to
CatalogTupleInsert and CatalogTupleUpdate); the only remaining one is in
InsertPgAttributeTuple. I propose that we inline the three lines into
all those places and just remove CatalogUpdateIndexes. Half the out-of-
core places that are using this function will be broken as soon as WARM
lands anyway. I see no reason to keep it. (I have already modified the
patch this way -- no need to resend).

Unless there are objections I will push this later this afternoon.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Alvaro Herrera wrote:

Unless there are objections I will push this later this afternoon.

Done. Let's get on with the show -- please post a rebased WARM.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 2017-01-31 14:10:01 -0300, Alvaro Herrera wrote:

Pavan Deolasee wrote:

Two new APIs added.

- CatalogInsertHeapAndIndex which does a simple_heap_insert followed by
catalog updates
- CatalogUpdateHeapAndIndex which does a simple_heap_update followed by
catalog updates

There are only a handful callers remain for simple_heap_insert/update after
this patch. They are typically working with already opened indexes and
hence I left them unchanged.

Hmm, I was thinking we would get rid of CatalogUpdateIndexes altogether.
Two of the callers are in the new routines (which I propose to rename to
CatalogTupleInsert and CatalogTupleUpdate); the only remaining one is in
InsertPgAttributeTuple. I propose that we inline the three lines into
all those places and just remove CatalogUpdateIndexes. Half the out-of-
core places that are using this function will be broken as soon as WARM
lands anyway. I see no reason to keep it. (I have already modified the
patch this way -- no need to resend).

Unless there are objections I will push this later this afternoon.

Hm, sorry for missing this earlier. I think CatalogUpdateIndexes() is
fairly widely used in extensions - it seems like a pretty harsh change
to not leave some backward compatibility layer in place.

Andres

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

Andres Freund wrote:

On 2017-01-31 14:10:01 -0300, Alvaro Herrera wrote:

Hmm, I was thinking we would get rid of CatalogUpdateIndexes altogether.
Two of the callers are in the new routines (which I propose to rename to
CatalogTupleInsert and CatalogTupleUpdate); the only remaining one is in
InsertPgAttributeTuple. I propose that we inline the three lines into
all those places and just remove CatalogUpdateIndexes. Half the out-of-
core places that are using this function will be broken as soon as WARM
lands anyway. I see no reason to keep it. (I have already modified the
patch this way -- no need to resend).

Unless there are objections I will push this later this afternoon.

Hm, sorry for missing this earlier. I think CatalogUpdateIndexes() is
fairly widely used in extensions - it seems like a pretty harsh change
to not leave some backward compatibility layer in place.

Yeah, I can put it back if there's pushback about the removal, but I
think it's going to break due to WARM anyway.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 2017-01-31 19:10:05 -0300, Alvaro Herrera wrote:

Andres Freund wrote:

On 2017-01-31 14:10:01 -0300, Alvaro Herrera wrote:

Hmm, I was thinking we would get rid of CatalogUpdateIndexes altogether.
Two of the callers are in the new routines (which I propose to rename to
CatalogTupleInsert and CatalogTupleUpdate); the only remaining one is in
InsertPgAttributeTuple. I propose that we inline the three lines into
all those places and just remove CatalogUpdateIndexes. Half the out-of-
core places that are using this function will be broken as soon as WARM
lands anyway. I see no reason to keep it. (I have already modified the
patch this way -- no need to resend).

Unless there are objections I will push this later this afternoon.

Hm, sorry for missing this earlier. I think CatalogUpdateIndexes() is
fairly widely used in extensions - it seems like a pretty harsh change
to not leave some backward compatibility layer in place.

Yeah, I can put it back if there's pushback about the removal, but I
think it's going to break due to WARM anyway.

I'm a bit doubtful (but not extremely so) that that's ok.

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

Andres Freund <andres@anarazel.de> writes:

Hm, sorry for missing this earlier. I think CatalogUpdateIndexes() is
fairly widely used in extensions - it seems like a pretty harsh change
to not leave some backward compatibility layer in place.

If an extension is doing that, it is probably constructing tuples to put
into the catalog, which means it'd be equally (and much more quietly)
broken by any change to the catalog's schema. We've never considered
such an argument as a reason not to change catalog schemas, though.

In short, I've got mighty little sympathy for that argument.

(I'm a little more concerned by Alvaro's apparent position that WARM
is a done deal; I didn't think so. This particular change seems like
good cleanup anyhow, however.)

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Andres Freund <andres@anarazel.de> writes:

Hm, sorry for missing this earlier. I think CatalogUpdateIndexes() is
fairly widely used in extensions - it seems like a pretty harsh change
to not leave some backward compatibility layer in place.

If an extension is doing that, it is probably constructing tuples to put
into the catalog, which means it'd be equally (and much more quietly)
broken by any change to the catalog's schema. We've never considered
such an argument as a reason not to change catalog schemas, though.

In short, I've got mighty little sympathy for that argument.

+1

(I'm a little more concerned by Alvaro's apparent position that WARM
is a done deal; I didn't think so. This particular change seems like
good cleanup anyhow, however.)

Agreed.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

(I'm a little more concerned by Alvaro's apparent position that WARM
is a done deal; I didn't think so. This particular change seems like
good cleanup anyhow, however.)

Agreed.

BTW, the reason I think it's good cleanup is that it's something that my
colleagues at Salesforce also had to do as part of putting PG on top of a
different storage engine that had different ideas about index handling.
Essentially it's providing a bit of abstraction as to whether catalog
storage is exactly heaps or not (a topic I've noticed Robert is starting
to take some interest in, as well). However, the patch misses an
important part of such an abstraction layer by not also converting
catalog-related simple_heap_delete() calls into some sort of
CatalogTupleDelete() operation. It is certainly a peculiarity of
PG heaps that deletions don't require any immediate index work --- most
other storage engines would need that.

I propose that we should finish the job by inventing CatalogTupleDelete(),
which for the moment would be a trivial wrapper around
simple_heap_delete(), maybe just a macro for it.

If there's no objections I'll go make that happen in a day or two.

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

On 2017-01-31 17:21:28 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Hm, sorry for missing this earlier. I think CatalogUpdateIndexes() is
fairly widely used in extensions - it seems like a pretty harsh change
to not leave some backward compatibility layer in place.

If an extension is doing that, it is probably constructing tuples to put
into the catalog, which means it'd be equally (and much more quietly)
broken by any change to the catalog's schema. We've never considered
such an argument as a reason not to change catalog schemas, though.

I know of several extensions that use CatalogUpdateIndexes() to update
their own tables. Citus included (It's trivial to change on our side, so
that's not a reason to do or not do something). There really is no
convenient API to do so without it.

(I'm a little more concerned by Alvaro's apparent position that WARM
is a done deal; I didn't think so. This particular change seems like
good cleanup anyhow, however.)

Yea, I don't think we're even close to that either.

Andres

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

Tom Lane wrote:

BTW, the reason I think it's good cleanup is that it's something that my
colleagues at Salesforce also had to do as part of putting PG on top of a
different storage engine that had different ideas about index handling.
Essentially it's providing a bit of abstraction as to whether catalog
storage is exactly heaps or not (a topic I've noticed Robert is starting
to take some interest in, as well).

Yeah, I remembered that too. Of course, we'd need to change the whole
idea of mapping tuples to C structs too, but this seemed a nice step
forward. (I renamed Pavan's proposed routine precisely to avoid the
word "Heap" in it.)

However, the patch misses an
important part of such an abstraction layer by not also converting
catalog-related simple_heap_delete() calls into some sort of
CatalogTupleDelete() operation. It is certainly a peculiarity of
PG heaps that deletions don't require any immediate index work --- most
other storage engines would need that.

I propose that we should finish the job by inventing CatalogTupleDelete(),
which for the moment would be a trivial wrapper around
simple_heap_delete(), maybe just a macro for it.

If there's no objections I'll go make that happen in a day or two.

Sounds good.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, Feb 1, 2017 at 9:36 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

I propose that we should finish the job by inventing CatalogTupleDelete(),
which for the moment would be a trivial wrapper around
simple_heap_delete(), maybe just a macro for it.

If there's no objections I'll go make that happen in a day or two.

Sounds good.

As you are on it, I have moved the patch to CF 2017-03.
--
Michael

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

On Tue, Jan 31, 2017 at 7:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

+#define HeapTupleHeaderGetNextTid(tup, next_ctid) \
+do { \
+     AssertMacro(!((tup)->t_infomask2 & HEAP_LATEST_TUPLE)); \
+     ItemPointerCopy(&(tup)->t_ctid, (next_ctid)); \
+} while (0)

Actually, I think this macro could just return the TID so that it can

be

used as struct assignment, just like ItemPointerCopy does internally --
callers can do
ctid = HeapTupleHeaderGetNextTid(tup);

While I agree with your proposal, I wonder why we have ItemPointerCopy()

in

the first place because we freely copy TIDs as struct assignment. Is

there

a reason for that? And if there is, does it impact this specific case?

I dunno. This macro is present in our very first commit d31084e9d1118b.
Maybe it's an artifact from the Lisp to C conversion. Even then, we had
some cases of iptrs being copied by struct assignment, so it's not like
it didn't work. Perhaps somebody envisioned that the internal details
could change, but that hasn't happened in two decades so why should we
worry about it now? If somebody needs it later, it can be changed then.

May I suggest in that case that we apply the attached patch which removes
all references to ItemPointerCopy and its definition as well? This will
avoid confusion in future too. No issues noticed in regression tests.

There is one issue that bothers me. The current implementation lacks
ability to convert WARM chains into HOT chains. The README.WARM has some
proposal to do that. But it requires additional free bit in tuple header
(which we don't have) and of course, it needs to be vetted and

implemented.

If the heap ends up with many WARM tuples, then index-only-scans will
become ineffective because index-only-scan can not skip a heap page, if

it

contains a WARM tuple. Alternate ideas/suggestions and review of the

design

are welcome!

t_infomask2 contains one last unused bit,

Umm, WARM is using 2 unused bits from t_infomask2. You mean there is
another free bit after that too?

and we could reuse vacuum
full's bits (HEAP_MOVED_OUT, HEAP_MOVED_IN), but that will need some
thinking ahead. Maybe now's the time to start versioning relations so
that we can ensure clusters upgraded to pg10 do not contain any of those
bits in any tuple headers.

Yeah, IIRC old VACUUM FULL was removed in 9.0, which is good 6 year old.
Obviously, there still a chance that a pre-9.0 binary upgraded cluster
exists and upgrades to 10. So we still need to do something about them if
we reuse these bits. I'm surprised to see that we don't have any mechanism
in place to clear those bits. So may be we add something to do that.

I'd some other ideas (and a patch too) to reuse bits from t_ctid.ip_pos
given that offset numbers can be represented in just 13 bits, even with the
maximum block size. Can look at that if it comes to finding more bits.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Tom Lane wrote:

However, the patch misses an
important part of such an abstraction layer by not also converting
catalog-related simple_heap_delete() calls into some sort of
CatalogTupleDelete() operation. It is certainly a peculiarity of
PG heaps that deletions don't require any immediate index work --- most
other storage engines would need that.
I propose that we should finish the job by inventing CatalogTupleDelete(),
which for the moment would be a trivial wrapper around
simple_heap_delete(), maybe just a macro for it.

If there's no objections I'll go make that happen in a day or two.

Sounds good.

So while I was working on this I got quite unhappy with the
already-committed patch: it's a leaky abstraction in more ways than
this, and it's created a possibly-serious performance regression
for large objects (and maybe other places).

The source of both of those problems is that in some places, we
did CatalogOpenIndexes and then used the CatalogIndexState for
multiple tuple inserts/updates before doing CatalogCloseIndexes.
The patch dealt with these either by not touching them, just
leaving the simple_heap_insert/update calls in place (thus failing
to create any abstraction), or by blithely ignoring the optimization
and doing s/simple_heap_insert/CatalogTupleInsert/ anyway. For example,
in inv_api.c we are now doing a CatalogOpenIndexes/CatalogCloseIndexes
cycle for each chunk of the large object ... and just to add insult to
injury, the now-useless open/close calls outside the loop are still there.

I think what we ought to do about this is invent additional API
functions, say

Oid CatalogTupleInsertWithInfo(Relation heapRel, HeapTuple tup,
CatalogIndexState indstate);
void CatalogTupleUpdateWithInfo(Relation heapRel, ItemPointer otid,
HeapTuple tup, CatalogIndexState indstate);

and use these in place of simple_heap_foo plus CatalogIndexInsert
in the places where this optimization had been applied.

An alternative but much more complicated fix would be to get rid of
the necessity for callers to worry about this at all, by caching
a CatalogIndexState in the catalog's relcache entry. That might be
worth doing eventually (because it would allow sharing index info
collection across unrelated operations) but I don't want to do it today.

Objections, better naming ideas?

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

Tom Lane wrote:

The source of both of those problems is that in some places, we
did CatalogOpenIndexes and then used the CatalogIndexState for
multiple tuple inserts/updates before doing CatalogCloseIndexes.
The patch dealt with these either by not touching them, just
leaving the simple_heap_insert/update calls in place (thus failing
to create any abstraction), or by blithely ignoring the optimization
and doing s/simple_heap_insert/CatalogTupleInsert/ anyway. For example,
in inv_api.c we are now doing a CatalogOpenIndexes/CatalogCloseIndexes
cycle for each chunk of the large object ... and just to add insult to
injury, the now-useless open/close calls outside the loop are still there.

Ouch. You're right, I missed that.

I think what we ought to do about this is invent additional API
functions, say

Oid CatalogTupleInsertWithInfo(Relation heapRel, HeapTuple tup,
CatalogIndexState indstate);
void CatalogTupleUpdateWithInfo(Relation heapRel, ItemPointer otid,
HeapTuple tup, CatalogIndexState indstate);

and use these in place of simple_heap_foo plus CatalogIndexInsert
in the places where this optimization had been applied.

This looks reasonable enough to me.

An alternative but much more complicated fix would be to get rid of
the necessity for callers to worry about this at all, by caching
a CatalogIndexState in the catalog's relcache entry. That might be
worth doing eventually (because it would allow sharing index info
collection across unrelated operations) but I don't want to do it today.

Hmm, interesting idea. No disagreement on postponing.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Tom Lane wrote:

I think what we ought to do about this is invent additional API
functions, say

Oid CatalogTupleInsertWithInfo(Relation heapRel, HeapTuple tup,
CatalogIndexState indstate);
void CatalogTupleUpdateWithInfo(Relation heapRel, ItemPointer otid,
HeapTuple tup, CatalogIndexState indstate);

and use these in place of simple_heap_foo plus CatalogIndexInsert
in the places where this optimization had been applied.

This looks reasonable enough to me.

Done.

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

On Thu, Feb 2, 2017 at 3:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Tom Lane wrote:

I think what we ought to do about this is invent additional API
functions, say

Oid CatalogTupleInsertWithInfo(Relation heapRel, HeapTuple tup,
CatalogIndexState indstate);
void CatalogTupleUpdateWithInfo(Relation heapRel, ItemPointer otid,
HeapTuple tup, CatalogIndexState indstate);

and use these in place of simple_heap_foo plus CatalogIndexInsert
in the places where this optimization had been applied.

This looks reasonable enough to me.

Done.

Thanks for taking care of this. Shame that I missed this because I'd
specifically noted the special casing for large objects etc. But looks like
while changing 180+ call sites, I forgot my notes.

Thanks again,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Feb 1, 2017 at 3:21 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Alvaro Herrera wrote:

Unless there are objections I will push this later this afternoon.

Done. Let's get on with the show -- please post a rebased WARM.

Please see rebased patches attached. There is not much change other than
the fact the patch now uses new catalog maintenance API.

Do you think we should apply the patch to remove ItemPointerCopy()? I will
rework the HeapTupleHeaderGetNextTid() after that. Not that it depends on
removing ItemPointerCopy(), but decided to postpone it until we make a call
on that patch.

BTW I've run now long stress tests with the patch applied and see no new
issues, even when indexes are dropped and recreated concurrently (includes
my patch to fix CIC bug in the master though). In another 24 hour test,
WARM could do 274M transactions where as master did 164M transactions. I
did not drop and recreate indexes during this run.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavan Deolasee wrote:

Do you think we should apply the patch to remove ItemPointerCopy()? I will
rework the HeapTupleHeaderGetNextTid() after that. Not that it depends on
removing ItemPointerCopy(), but decided to postpone it until we make a call
on that patch.

My inclination is not to. We don't really know where we are going with
storage layer reworks in the near future, and we might end up changing
this in other ways. We might find ourselves needing this kind of
abstraction again. I don't think this means we need to follow it
completely in new code, since it's already broken in other places, but
let's not destroy it completely just yet.

BTW I've run now long stress tests with the patch applied and see no new
issues, even when indexes are dropped and recreated concurrently (includes
my patch to fix CIC bug in the master though). In another 24 hour test,
WARM could do 274M transactions where as master did 164M transactions. I
did not drop and recreate indexes during this run.

Eh, that's a 67% performance improvement. Nice.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Thu, Feb 2, 2017 at 6:17 PM, Pavan Deolasee <pavan.deolasee@gmail.com>
wrote:

Please see rebased patches attached. There is not much change other than
the fact the patch now uses new catalog maintenance API.

Another rebase on current master.

This time I am also attaching a proof-of-concept patch to demonstrate chain
conversion. The proposed algorithm is mentioned in the README.WARM, but
I'll briefly explain here.

The chain conversion works in two phases and requires another index pass
during vacuum. During first heap scan, we collect candidate chains for
conversion. A chain qualifies for conversion if it has all tuples with
matching index keys with respect to all current indexes (i.e. chain becomes
HOT). WARM chains become HOT as and when old versions retire (or new
versions retire in case of aborts). But before we can mark them HOT again,
we must first remove duplicate (and potentially wrong) index pointers. This
algorithm deals with that.

When a WARM update occurs and we insert a new index entry in one or more
indexes, we mark the new index pointer with a special RED flag. The heap
tuple created by this UPDATE is also marked as RED. If the tuple is then
HOT-updated, subsequent versions will be marked RED as well. IOW each WARM
chain has two HOT chains inside it and these chains are identified as BLUE
and RED chains. The index pointer which satisfies key in RED chain is
marked RED too.

When we collect candidate WARM chains in the first heap scan, we also
remember the color of the chain.

During first index scan we delete all known dead index pointers (same as
lazy_tid_reaped). Also we also count number of RED and BLUE pointers to
each candidate chain.

The next index scan will either 1. remove an index pointer which is known
to be useless or 2. color a RED pointer BLUE.
- A BLUE pointer to a RED chain is removed when there exists a RED pointer
to the chain. If there is no RED pointer, we can't remove the BLUE pointer
because that is the only path to the heap tuple (case when WARM does not
cause new index entry). But we instead color the heap tuples BLUE
- A BLUE pointer to a BLUE chain is always retained
- A RED pointer to a BLUE chain is always removed (aborted updates)
- A RED pointer to a RED chain is colored BLUE (we will color the heap
tuples BLUE in the second heap scan)

Once the index pointers are taken care of such that there exists exactly
one pointer to a chain, the chain can be converted into HOT chains by
clearing WARM and RED flags.

There is one case of aborted vacuums. If a crash happens after coloring RED
pointer BLUE, but before we can clear the heap tuples, we might end up with
two BLUE pointers to a RED chain. This case will require recheck logic and
is not yet implemented.

The POC only works with BTREEs because the unused bit in IndexTuple's
t_info is already used by HASH indexes. For heap tuples, we can reuse one
of HEAP_MOVED_IN/OFF bits for marking tuples RED since this is only
required for WARM tuples. So the bit can be checked along with WARM bit.

Unless there is an objection to the design or someone thinks it cannot
work, I'll look at some alternate mechanism to free up more bits in tuple
header or at least in the index tuples. One idea is to free up 3 bits from
ip_posid knowing that OffsetNumber can never really need more than 13 bits
with the other constraints in place. We could use some bit-field magic to
do that with minimal changes. The thing that concerns me is whether there
will be a guaranteed way to make that work on all hardwares without
breaking the on-disk layout.

Comments/suggestions?

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi Tom,

On Wed, Feb 1, 2017 at 3:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

(I'm a little more concerned by Alvaro's apparent position that WARM
is a done deal; I didn't think so.

Are there any specific aspects of the design that you're not comfortable
with? I'm sure there could be some rough edges in the implementation that
I'm hoping will get handled during the further review process. But if there
are some obvious things I'm overlooking please let me know.

Probably the same question to Andres/Robert who has flagged concerns. On my
side, I've run some very long tests with data validation and haven't found
any new issues with the most recent patches.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Jan 31, 2017 at 04:52:39PM +0530, Pavan Deolasee wrote:

The other critical bug I found, which unfortunately exists in the master too,
is the index corruption during CIC. The patch includes the same fix that I've
proposed on the other thread. With these changes, WARM stress is running fine
for last 24 hours on a decently powerful box. Multiple CREATE/DROP INDEX cycles
and updates via different indexed columns, with a mix of FOR SHARE/UPDATE and
rollbacks did not produce any consistency issues. A side note: while
performance measurement wasn't a goal of stress tests, WARM has done about 67%
more transaction than master in 24 hour period (95M in master vs 156M in WARM
to be precise on a 30GB table including indexes). I believe the numbers would
be far better had the test not dropping and recreating the indexes, thus
effectively cleaning up all index bloats. Also the table is small enough to fit
in the shared buffers. I'll rerun these tests with much larger scale factor and
without dropping indexes.

Thanks for setting up the test harness. I know it is hard but
in this case it has found an existing bug and given good performance
numbers. :-)

I have what might be a supid question. As I remember, WARM only allows
a single index-column change in the chain. Why are you seeing such a
large performance improvement? I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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

On Wed, Feb 1, 2017 at 10:46:45AM +0530, Pavan Deolasee wrote:

contains a WARM tuple. Alternate ideas/suggestions and review of the

design

are welcome!

t_infomask2 contains one last unused bit,

Umm, WARM is using 2 unused bits from t_infomask2. You mean there is another
free bit after that too?

We are obviously going to use several heap or item pointer bits for
WARM, and once we do that it is going to be hard to undo that. Pavan,
are you saying you could do more with WARM if you had more bits? Are we
sure we have given you all the bits we can? Do we want to commit to a
lesser feature because the bits are not available?

and we could reuse vacuum
full's bits (HEAP_MOVED_OUT, HEAP_MOVED_IN), but that will need some
thinking ahead.  Maybe now's the time to start versioning relations so
that we can ensure clusters upgraded to pg10 do not contain any of those
bits in any tuple headers.

Yeah, IIRC old VACUUM FULL was removed in 9.0, which is good 6 year old.
Obviously, there still a chance that a pre-9.0 binary upgraded cluster exists
and upgrades to 10. So we still need to do something about them if we reuse
these bits. I'm surprised to see that we don't have any mechanism in place to
clear those bits. So may be we add something to do that.

Yeah, good question. :-( We have talked about adding some page,
table, or cluster-level version number so we could identify if a given
tuple _could_ be using those bits, but never did it.

I'd some other ideas (and a patch too) to reuse bits from t_ctid.ip_pos given
that offset numbers can be represented in just 13 bits, even with the maximum
block size. Can look at that if it comes to finding more bits.

OK, so it seems more bits is not a blocker to enhancements, yet.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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

Bruce Momjian wrote:

As I remember, WARM only allows
a single index-column change in the chain. Why are you seeing such a
large performance improvement? I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

The second update in a chain creates another non-warm-updated tuple, so
the third update can be a warm update again, and so on.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Thu, Feb 23, 2017 at 03:03:39PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

As I remember, WARM only allows
a single index-column change in the chain. Why are you seeing such a
large performance improvement? I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

The second update in a chain creates another non-warm-updated tuple, so
the third update can be a warm update again, and so on.

Right, before this patch they would be two independent HOT chains. It
still seems like an unexpectedly-high performance win. Are two
independent HOT chains that much more expensive than joining them via
WARM?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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

Bruce Momjian wrote:

On Thu, Feb 23, 2017 at 03:03:39PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

As I remember, WARM only allows
a single index-column change in the chain. Why are you seeing such a
large performance improvement? I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

The second update in a chain creates another non-warm-updated tuple, so
the third update can be a warm update again, and so on.

Right, before this patch they would be two independent HOT chains.

No, they would be a regular update chain, not HOT updates.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Thu, Feb 23, 2017 at 03:26:09PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

On Thu, Feb 23, 2017 at 03:03:39PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

As I remember, WARM only allows
a single index-column change in the chain. Why are you seeing such a
large performance improvement? I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

The second update in a chain creates another non-warm-updated tuple, so
the third update can be a warm update again, and so on.

Right, before this patch they would be two independent HOT chains.

No, they would be a regular update chain, not HOT updates.

Well, let's walk through this. Let's suppose you have three updates
that stay on the same page and don't update any indexed columns --- that
would produce a HOT chain of four tuples. If you then do an update that
changes an indexed column, prior to this patch, you get a normal update,
and more HOT updates can be added to this. With WARM, we can join those
chains and potentially trim the first HOT chain as those tuples become
invisible.

Am I missing something?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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

Bruce Momjian wrote:

Well, let's walk through this. Let's suppose you have three updates
that stay on the same page and don't update any indexed columns --- that
would produce a HOT chain of four tuples. If you then do an update that
changes an indexed column, prior to this patch, you get a normal update,
and more HOT updates can be added to this. With WARM, we can join those
chains

With WARM, what happens is that the first three updates are HOT updates
just like currently, and the fourth one is a WARM update.

and potentially trim the first HOT chain as those tuples become
invisible.

That can already happen even without WARM, no?

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Thu, Feb 23, 2017 at 03:45:24PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

Well, let's walk through this. Let's suppose you have three updates
that stay on the same page and don't update any indexed columns --- that
would produce a HOT chain of four tuples. If you then do an update that
changes an indexed column, prior to this patch, you get a normal update,
and more HOT updates can be added to this. With WARM, we can join those
chains

With WARM, what happens is that the first three updates are HOT updates
just like currently, and the fourth one is a WARM update.

Right.

and potentially trim the first HOT chain as those tuples become
invisible.

That can already happen even without WARM, no?

Uh, the point is that with WARM those four early tuples can be removed
via a prune, rather than requiring a VACUUM. Without WARM, the fourth
tuple can't be removed until the index is cleared by VACUUM.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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

Bruce Momjian wrote:

On Thu, Feb 23, 2017 at 03:45:24PM -0300, Alvaro Herrera wrote:

and potentially trim the first HOT chain as those tuples become
invisible.

That can already happen even without WARM, no?

Uh, the point is that with WARM those four early tuples can be removed
via a prune, rather than requiring a VACUUM. Without WARM, the fourth
tuple can't be removed until the index is cleared by VACUUM.

I *think* that the WARM-updated one cannot be pruned either, because
it's pointed to by at least one index (otherwise it'd have been a HOT
update). The ones prior to that can be removed either way.

I think the part you want (be able to prune the WARM updated tuple) is
part of what Pavan calls "turning the WARM chain into a HOT chain", so
not part of the initial patch. Pavan can explain this part better, and
also set me straight in case I'm wrong in the above :-)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Thu, Feb 23, 2017 at 03:58:59PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

On Thu, Feb 23, 2017 at 03:45:24PM -0300, Alvaro Herrera wrote:

and potentially trim the first HOT chain as those tuples become
invisible.

That can already happen even without WARM, no?

Uh, the point is that with WARM those four early tuples can be removed
via a prune, rather than requiring a VACUUM. Without WARM, the fourth
tuple can't be removed until the index is cleared by VACUUM.

I *think* that the WARM-updated one cannot be pruned either, because
it's pointed to by at least one index (otherwise it'd have been a HOT
update). The ones prior to that can be removed either way.

Well, if you can't prune across index-column changes, how is a WARM
update different than just two HOT chains with no WARM linkage?

I think the part you want (be able to prune the WARM updated tuple) is
part of what Pavan calls "turning the WARM chain into a HOT chain", so
not part of the initial patch. Pavan can explain this part better, and
also set me straight in case I'm wrong in the above :-)

VACUUM can already remove entire HOT chains that have expired. What
his VACUUM patch does, I think, is to remove the index entries that no
longer point to values in the HOT/WARM chain, turning the chain into
fully HOT, so another WARM addition to the chain can happen.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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

On Thu, Feb 23, 2017 at 11:30 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Wed, Feb 1, 2017 at 10:46:45AM +0530, Pavan Deolasee wrote:

contains a WARM tuple. Alternate ideas/suggestions and review of

the

design

are welcome!

t_infomask2 contains one last unused bit,

Umm, WARM is using 2 unused bits from t_infomask2. You mean there is

another

free bit after that too?

We are obviously going to use several heap or item pointer bits for
WARM, and once we do that it is going to be hard to undo that. Pavan,
are you saying you could do more with WARM if you had more bits? Are we
sure we have given you all the bits we can? Do we want to commit to a
lesser feature because the bits are not available?

btree implementation is complete as much as I would like (there are a few
TODOs, but no show stoppers), at least for the first release. There is a
free bit in btree index tuple header that I could use for chain conversion.
In the heap tuples, I can reuse HEAP_MOVED_OFF because that bit will only
be set along with HEAP_WARM_TUPLE bit. Since none of the upgraded clusters
can have HEAP_WARM_TUPLE bit set, I think we are safe.

WARM currently also supports hash indexes, but there is no free bit left in
hash index tuple header. I think I can work around that by using a bit from
ip_posid (not yet implemented/tested, but seems doable).

IMHO if we can do that i.e. support btree and hash indexes to start with,
we should be good to go for the first release. We can try to support other
popular index AMs in the subsequent release.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Feb 23, 2017 at 9:21 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Tue, Jan 31, 2017 at 04:52:39PM +0530, Pavan Deolasee wrote:

The other critical bug I found, which unfortunately exists in the master too,
is the index corruption during CIC. The patch includes the same fix that I've
proposed on the other thread. With these changes, WARM stress is running fine
for last 24 hours on a decently powerful box. Multiple CREATE/DROP INDEX cycles
and updates via different indexed columns, with a mix of FOR SHARE/UPDATE and
rollbacks did not produce any consistency issues. A side note: while
performance measurement wasn't a goal of stress tests, WARM has done about 67%
more transaction than master in 24 hour period (95M in master vs 156M in WARM
to be precise on a 30GB table including indexes). I believe the numbers would
be far better had the test not dropping and recreating the indexes, thus
effectively cleaning up all index bloats. Also the table is small enough to fit
in the shared buffers. I'll rerun these tests with much larger scale factor and
without dropping indexes.

Thanks for setting up the test harness. I know it is hard but
in this case it has found an existing bug and given good performance
numbers. :-)

I have what might be a supid question. As I remember, WARM only allows
a single index-column change in the chain. Why are you seeing such a
large performance improvement? I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

I'm not sure how the test case is set up. If the table has multiple
indexes, each on a different column, and only one of the indexes is
updated, then you figure to win because now the other indexes need
less maintenance (and get less bloated). If you have only a single
index, then I don't see how WARM can be any better than HOT, but maybe
I just don't understand the situation.

--
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

On Thu, Feb 23, 2017 at 11:53 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Thu, Feb 23, 2017 at 03:03:39PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

As I remember, WARM only allows
a single index-column change in the chain. Why are you seeing such a
large performance improvement? I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

The second update in a chain creates another non-warm-updated tuple, so
the third update can be a warm update again, and so on.

Right, before this patch they would be two independent HOT chains. It
still seems like an unexpectedly-high performance win. Are two
independent HOT chains that much more expensive than joining them via
WARM?

In these tests, there are zero HOT updates, since every update modifies
some index column. With WARM, we could reduce regular updates to half, even
when we allow only one WARM update per chain (chain really has a single
tuple for this discussion). IOW approximately half updates insert new index
entry in *every* index and half updates
insert new index entry *only* in affected index. That itself does a good
bit for performance.

So to answer your question: yes, joining two HOT chains via WARM is much
cheaper because it results in creating new index entries just for affected
indexes.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Feb 24, 2017 at 2:13 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Feb 23, 2017 at 9:21 PM, Bruce Momjian <bruce@momjian.us> wrote:

I have what might be a supid question. As I remember, WARM only allows
a single index-column change in the chain. Why are you seeing such a
large performance improvement? I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

I'm not sure how the test case is set up. If the table has multiple
indexes, each on a different column, and only one of the indexes is
updated, then you figure to win because now the other indexes need
less maintenance (and get less bloated). If you have only a single
index, then I don't see how WARM can be any better than HOT, but maybe
I just don't understand the situation.

That's correct. If you have just one index and if the UPDATE modifies
indexed indexed, the UPDATE won't be a WARM update and the patch gives you
no benefit. OTOH if the UPDATE doesn't modify any indexed columns, then it
will be a HOT update and again the patch gives you no benefit. It might be
worthwhile to see if patch causes any regression in these scenarios, though
I think it will be minimal or zero.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Feb 24, 2017 at 12:28 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Bruce Momjian wrote:

On Thu, Feb 23, 2017 at 03:45:24PM -0300, Alvaro Herrera wrote:

and potentially trim the first HOT chain as those tuples become
invisible.

That can already happen even without WARM, no?

Uh, the point is that with WARM those four early tuples can be removed
via a prune, rather than requiring a VACUUM. Without WARM, the fourth
tuple can't be removed until the index is cleared by VACUUM.

I *think* that the WARM-updated one cannot be pruned either, because
it's pointed to by at least one index (otherwise it'd have been a HOT
update). The ones prior to that can be removed either way.

No, even the WARM-updated can be pruned and if there are further HOT
updates, those can be pruned too. All indexes and even multiple pointers
from the same index are always pointing to the root of the WARM chain and
that line pointer does not go away unless the entire chain become dead. The
only material difference between HOT and WARM is that since there are two
index pointers from the same index to the same root line pointer, we must
do recheck. But HOT-pruning and all such things remain the same.

Let's take an example. Say, we have a table (a int, b int, c text) and two
indexes on first two columns.

H W
H
(1, 100, 'foo') -----> (1, 100, 'bar') ------> (1, 200, 'bar') -----> (1,
200, 'foo')

The first update will be a HOT update, the second update will be a WARM
update and the third update will again be a HOT update. The first and third
update do not create any new index entry, though the second update will
create a new index entry in the second index. Any further WARM updates to
this chain is not allowed, but further HOT updates are ok.

If all but the last version become DEAD, HOT-prune will remove all of them
and turn the first line pointer into REDIRECT line pointer. At this point,
the first index has one index pointer and the second index has two index
pointers, but all pointing to the same root line pointer, which has not
become REDIRECT line pointer.

Redirect
o-----------------------> (1, 200, 'foo')

I think the part you want (be able to prune the WARM updated tuple) is

part of what Pavan calls "turning the WARM chain into a HOT chain", so
not part of the initial patch. Pavan can explain this part better, and
also set me straight in case I'm wrong in the above :-)

Umm.. it's a bit different. Without chain conversion, we still don't allow
further WARM updates to the above chain because that might create a third
index pointer and our recheck logic can't cope up with duplicate scans. HOT
updates are allowed though.

The latest patch that I proposed will handle this case and convert such
chains into regular HOT-pruned chains. To do that, we must remove the
duplicate (and now wrong) index pointer to the chain. Once we do that and
change the state on the heap tuple, we can once again do WARM update to
this tuple. Note that in this example the chain has just one tuple, which
will be the case typically, but the algorithm can deal with the case where
there are multiple tuples but with matching index keys.

Hope this helps.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Feb 24, 2017 at 2:42 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

Let's take an example. Say, we have a table (a int, b int, c text) and two
indexes on first two columns.

H W
H
(1, 100, 'foo') -----> (1, 100, 'bar') ------> (1, 200, 'bar') -----> (1,
200, 'foo')

The first update will be a HOT update, the second update will be a WARM
update and the third update will again be a HOT update. The first and third
update do not create any new index entry, though the second update will
create a new index entry in the second index. Any further WARM updates to
this chain is not allowed, but further HOT updates are ok.

If all but the last version become DEAD, HOT-prune will remove all of them
and turn the first line pointer into REDIRECT line pointer.

So, when you do the WARM update, the new index entries still point at
the original root, which they don't match, not the version where that
new value first appeared?

I don't immediately see how this will work with index-only scans. If
the tuple is HOT updated several times, HOT-pruned back to a single
version, and then the page is all-visible, the index entries are
guaranteed to agree with the remaining tuple, so it's fine to believe
the data in the index tuple. But with WARM, that would no longer be
true, unless you have some trick for that...

--
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

On Fri, Feb 24, 2017 at 3:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I don't immediately see how this will work with index-only scans. If
the tuple is HOT updated several times, HOT-pruned back to a single
version, and then the page is all-visible, the index entries are
guaranteed to agree with the remaining tuple, so it's fine to believe
the data in the index tuple. But with WARM, that would no longer be
true, unless you have some trick for that...

Well the trick is to not allow index-only scans on such pages by not
marking them all-visible. That's why when a tuple is WARM updated, we carry
that information in the subsequent versions even when later updates are HOT
updates. The chain conversion algorithm will handle this by clearing those
bits and thus allowing index-only scans again.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Feb 24, 2017 at 3:31 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

On Fri, Feb 24, 2017 at 3:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I don't immediately see how this will work with index-only scans. If
the tuple is HOT updated several times, HOT-pruned back to a single
version, and then the page is all-visible, the index entries are
guaranteed to agree with the remaining tuple, so it's fine to believe
the data in the index tuple. But with WARM, that would no longer be
true, unless you have some trick for that...

Well the trick is to not allow index-only scans on such pages by not marking
them all-visible. That's why when a tuple is WARM updated, we carry that
information in the subsequent versions even when later updates are HOT
updates. The chain conversion algorithm will handle this by clearing those
bits and thus allowing index-only scans again.

Wow, OK. In my view, that makes the chain conversion code pretty much
essential, because if you had WARM without chain conversion then the
visibility map gets more or less irrevocably less effective over time,
which sounds terrible. But it sounds to me like even with the chain
conversion, it might take multiple vacuum passes before all visibility
map bits are set, which isn't such a great property (thus e.g.
fdf9e21196a6f58c6021c967dc5776a16190f295).

--
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

On Fri, Feb 24, 2017 at 3:42 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Wow, OK. In my view, that makes the chain conversion code pretty much
essential, because if you had WARM without chain conversion then the
visibility map gets more or less irrevocably less effective over time,
which sounds terrible.

Yes. I decided to complete chain conversion patch when I realised that IOS
will otherwise become completely useful if large percentage of rows are
updated just once. So I agree. It's not an optional patch and should get in
with the main WARM patch.

But it sounds to me like even with the chain
conversion, it might take multiple vacuum passes before all visibility
map bits are set, which isn't such a great property (thus e.g.
fdf9e21196a6f58c6021c967dc5776a16190f295).

The chain conversion algorithm first converts the chains during vacuum and
then checks if the page can be set all-visible. So I'm not sure why it
would take multiple vacuums before a page is set all-visible. The commit
you quote was written to ensure that we make another attempt to set the
page all-visible after al dead tuples are removed from the page. Similarly,
we will convert all WARM chains to HOT chains and then check for
all-visibility of the page.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Feb 24, 2017 at 4:06 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

Wow, OK. In my view, that makes the chain conversion code pretty much
essential, because if you had WARM without chain conversion then the
visibility map gets more or less irrevocably less effective over time,
which sounds terrible.

Yes. I decided to complete chain conversion patch when I realised that IOS
will otherwise become completely useful if large percentage of rows are
updated just once. So I agree. It's not an optional patch and should get in
with the main WARM patch.

Right, and it's not just index-only scans. VACUUM gets permanently
more expensive, too, which is probably a much worse problem.

But it sounds to me like even with the chain
conversion, it might take multiple vacuum passes before all visibility
map bits are set, which isn't such a great property (thus e.g.
fdf9e21196a6f58c6021c967dc5776a16190f295).

The chain conversion algorithm first converts the chains during vacuum and
then checks if the page can be set all-visible. So I'm not sure why it would
take multiple vacuums before a page is set all-visible. The commit you quote
was written to ensure that we make another attempt to set the page
all-visible after al dead tuples are removed from the page. Similarly, we
will convert all WARM chains to HOT chains and then check for all-visibility
of the page.

OK, that sounds good. And there are no bugs, right? :-)

--
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

On Fri, Feb 24, 2017 at 02:14:23PM +0530, Pavan Deolasee wrote:

On Thu, Feb 23, 2017 at 11:53 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Thu, Feb 23, 2017 at 03:03:39PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

As I remember, WARM only allows
a single index-column change in the chain.  Why are you seeing such a
large performance improvement?  I would have thought it would be that
high if we allowed an unlimited number of index changes in the chain.

The second update in a chain creates another non-warm-updated tuple, so
the third update can be a warm update again, and so on.

Right, before this patch they would be two independent HOT chains.  It
still seems like an unexpectedly-high performance win.  Are two
independent HOT chains that much more expensive than joining them via
WARM?

In these tests, there are zero HOT updates, since every update modifies some
index column. With WARM, we could reduce regular updates to half, even when we
allow only one WARM update per chain (chain really has a single tuple for this
discussion). IOW approximately half updates insert new index entry in *every*
index and half updates 
insert new index entry *only* in affected index. That itself does a good bit
for performance.

So to answer your question: yes, joining two HOT chains via WARM is much
cheaper because it results in creating new index entries just for affected
indexes.

OK, all my questions have been answered, including the use of flag bits.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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

On Fri, Feb 24, 2017 at 9:47 PM, Robert Haas <robertmhaas@gmail.com> wrote:

And there are no bugs, right? :-)

Yeah yeah absolutely nothing. Just like any other feature committed to
Postgres so far ;-)

I need to polish the chain conversion patch a bit and also add missing
support for redo, hash indexes etc. Support for hash indexes will need
overloading of ip_posid bits in the index tuple (since there are no free
bits left in hash tuples). I plan to work on that next and submit a fully
functional patch, hopefully before the commit-fest starts.

(I have mentioned the idea of overloading ip_posid bits a few times now and
haven't heard any objection so far. Well, that could either mean that
nobody has read those emails seriously or there is general acceptance to
that idea.. I am assuming latter :-))

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Feb 25, 2017 at 10:50:57AM +0530, Pavan Deolasee wrote:

On Fri, Feb 24, 2017 at 9:47 PM, Robert Haas <robertmhaas@gmail.com> wrote:
And there are no bugs, right?  :-)

Yeah yeah absolutely nothing. Just like any other feature committed to Postgres
so far ;-)

I need to polish the chain conversion patch a bit and also add missing support
for redo, hash indexes etc. Support for hash indexes will need overloading of
ip_posid bits in the index tuple (since there are no free bits left in hash
tuples). I plan to work on that next and submit a fully functional patch,
hopefully before the commit-fest starts.

(I have mentioned the idea of overloading ip_posid bits a few times now and
haven't heard any objection so far. Well, that could either mean that nobody
has read those emails seriously or there is general acceptance to that idea.. I
am assuming latter :-))

Yes, I think it is the latter.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

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

On Sat, Feb 25, 2017 at 10:50 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

On Fri, Feb 24, 2017 at 9:47 PM, Robert Haas <robertmhaas@gmail.com> wrote:

And there are no bugs, right? :-)

Yeah yeah absolutely nothing. Just like any other feature committed to
Postgres so far ;-)

Fair point, but I've already said why I think the stakes for this
particular feature are pretty high.

I need to polish the chain conversion patch a bit and also add missing
support for redo, hash indexes etc. Support for hash indexes will need
overloading of ip_posid bits in the index tuple (since there are no free
bits left in hash tuples). I plan to work on that next and submit a fully
functional patch, hopefully before the commit-fest starts.

(I have mentioned the idea of overloading ip_posid bits a few times now and
haven't heard any objection so far. Well, that could either mean that nobody
has read those emails seriously or there is general acceptance to that
idea.. I am assuming latter :-))

I'm not sure about that. I'm not really sure I have an opinion on
that yet, without seeing the patch. The discussion upthread was a bit
vague:

"One idea is to free up 3 bits from ip_posid knowing that OffsetNumber
can never really need more than 13 bits with the other constraints in
place."

Not sure exactly what "the other constraints" are, exactly.

/me goes off, tries to figure it out.

If I'm reading the definition of MaxIndexTuplesPerPage correctly, it
thinks that the minimum number of bytes per index tuple is at least
16: I think sizeof(IndexTupleData) will be 8, so when you add 1 and
MAXALIGN, you get to 12, and then ItemIdData is another 4. So an 8k
page (2^13 bits) could have, on a platform with MAXIMUM_ALIGNOF == 4,
as many as 2^9 tuples. To store more than 2^13 tuples, we'd need a
block size > 128k, but it seems 32k is the most we support. So that
seems OK, if I haven't gotten confused about the logic.

I suppose the only other point of concern about stealing some bits
there is that it might make some operations a little more expensive,
because they've got to start masking out the high bits. But that's
*probably* negligible.

--
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

On Sun, Feb 26, 2017 at 2:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Fair point, but I've already said why I think the stakes for this
particular feature are pretty high.

I understand your concerns and not trying to downplay them. I'm doing my
best to test the patch in different ways to ensure we can catch most of the
bugs before the patch is committed. Hopefully with additional reviews and
tests we can plug remaining holes, if any, and be in a comfortable state.

(I have mentioned the idea of overloading ip_posid bits a few times now

and

haven't heard any objection so far. Well, that could either mean that

nobody

has read those emails seriously or there is general acceptance to that
idea.. I am assuming latter :-))

I'm not sure about that. I'm not really sure I have an opinion on
that yet, without seeing the patch. The discussion upthread was a bit
vague:

Attached is a complete set of rebased and finished patches. Patches 0002
and 0003 does what I've in mind as far as OffsetNumber bits.

AFAICS this version is a fully functional implementation of WARM, ready for
serious review/test. The chain conversion is now fully functional and
tested with btrees. I've also added support for chain conversion in hash
indexes by overloading ip_posid high order bits. Even though there is a
free bit available in btree index tuple, the patch now uses the same
ip_posid bit even for btree indexes.

A short summary of all attached patches.

0000_interesting_attrs_v15.patch:

This is Alvaro's patch to refactor HeapSatisfiesHOTandKeyUpdate. We now
return a set of modified attributes and let the caller consume that
information in a way it wants. The main WARM patch uses this refactored API.

0001_track_root_lp_v15.patch:

This implements the logic to store the root offset of the HOT chain in the
t_ctid.ip_posid field. We use a free bit in heap tuple header to mark that
a particular tuple is at the end of the chain and store the root offset in
the ip_posid. For pg_upgraded clusters, this information could be missing
and we do the hard-work of going through the page tuples to find the root
offset.

0002_clear_ip_posid_blkid_refs_v15.patch:

This is mostly a cleanup patch which removes direct references to ip_posid
and ip_blkid from various places and replace them with appropriate
ItemPointer[Get|Set][Offset|Block]Number macros.

0003_freeup_3bits_ip_posid_v15.patch:

This patch frees up the high order 3 bits from ip_posid and makes them
available for other uses. As noted, we only need 13 bits to represent
OffsetNumber and hence the high order bits are unused. This patch should
only be applied along with 0002_clear_ip_posid_blkid_refs_v15.patch

0004_warm_updates_v15.patch:

This implements the main WARM logic, except for chain conversion (which is
implemented in the last patch of the series). It uses another free bit in
the heap tuple header to identify the WARM tuples. When the first WARM
update happens, the old and new versions of the tuple are marked with this
flag. All subsequent HOT tuples in the chain are also marked with this flag
so we never lose information about WARM updates, irrespective of whether it
commits or aborts. We then implement recheck logic to decide which index
pointer should return a tuple from the HOT chain.

WARM is currently supported for hash and btree indexes. If a table has an
index of any other type, WARM is disabled.

0005_warm_chain_conversion_v15.patch:

This patch implements the WARM chain conversion as discussed upthread and
also noted in the README.WARM. This patch requires yet another bit in the
heap tuple header. But since the bit is only set along with the
HEAP_WARM_TUPLE bit, we can safely reuse HEAP_MOVED_OFF bit for this
purpose. We also need a bit to distinguish two copies of index pointers to
know which pointer points to the pre-WARM-update HOT chain (Blue chain) and
which pointer points to post-WARM-update HOT chain (Red chain). We steal
this bit from t_tid.ip_posid field in the index tuple headers. As part of
this patch, I moved XLOG_HEAP2_MULTI_INSERT to RM_HEAP_ID (and renamed it
to XLOG_HEAP_MULTI_INSERT). While it's not necessary, I thought it will
allow us to restrict XLOG_HEAP_INIT_PAGE to RM_HEAP_ID and make that bit
available to define additional opcodes in RM_HEAD2_ID.

I've done some elaborate tests with these patches applied. I've primarily
used make-world, pgbench with additional indexes and the WARM stress test
(which was useful in catching CIC bug) to test the feature. While it does
not mean there are no additional bugs, all bugs that were known to me are
fixed in this version. I'll continue to run more tests, especially around
crash recovery, when indexes are dropped and recreated and also do more
performance tests.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Here's a rebased set of patches. This is the same Pavan posted; I only
fixed some whitespace and a trivial conflict in indexam.c, per 9b88f27cb42f.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, Mar 8, 2017 at 12:00 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Here's a rebased set of patches. This is the same Pavan posted; I only
fixed some whitespace and a trivial conflict in indexam.c, per 9b88f27cb42f.

No attachments.

--
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

Alvaro Herrera wrote:

Here's a rebased set of patches. This is the same Pavan posted; I only
fixed some whitespace and a trivial conflict in indexam.c, per 9b88f27cb42f.

Jaime noted that I forgot the attachments. Here they are

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Mar 8, 2017 at 12:14 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Alvaro Herrera wrote:

Here's a rebased set of patches. This is the same Pavan posted; I only
fixed some whitespace and a trivial conflict in indexam.c, per 9b88f27cb42f.

Jaime noted that I forgot the attachments. Here they are

If I recall correctly, the main concern about 0001 was whether it
might negatively affect performance, and testing showed that, if
anything, it was a little better. Does that sound right?

Regarding 0002, I think this could use some documentation someplace
explaining the overall theory of operation. README.HOT, maybe?

+     * Most often and unless we are dealing with a pg-upgraded cluster, the
+     * root offset information should be cached. So there should not be too
+     * much overhead of fetching this information. Also, once a tuple is
+     * updated, the information will be copied to the new version. So it's not
+     * as if we're going to pay this price forever.

What if a tuple is updated -- presumably clearing the
HEAP_LATEST_TUPLE on the tuple at the end of the chain -- and then the
update aborts? Then we must be back to not having this information.

One overall question about this patch series is how we feel about
using up this many bits. 0002 uses a bit from infomask, and 0005 uses
a bit from infomask2. I'm not sure if that's everything, and then I
think we're steeling some bits from the item pointers, too. While the
performance benefits of the patch sound pretty good based on the test
results so far, this is definitely the very last time we'll be able to
implement a feature that requires this many bits.

--
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

Robert Haas wrote:

On Wed, Mar 8, 2017 at 12:14 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Alvaro Herrera wrote:

Here's a rebased set of patches. This is the same Pavan posted; I only
fixed some whitespace and a trivial conflict in indexam.c, per 9b88f27cb42f.

Jaime noted that I forgot the attachments. Here they are

If I recall correctly, the main concern about 0001 was whether it
might negatively affect performance, and testing showed that, if
anything, it was a little better. Does that sound right?

Not really -- it's a bit slower actually in a synthetic case measuring
exactly the slowed-down case. See
/messages/by-id/CAD__OugK12ZqMWWjZiM-YyuD1y8JmMy6x9YEctNiF3rPp6hy0g@mail.gmail.com
I bet in normal cases it's unnoticeable. If WARM flies, then it's going
to provide a larger improvement than is lost to this.

Regarding 0002, I think this could use some documentation someplace
explaining the overall theory of operation. README.HOT, maybe?

Hmm. Yeah, we should have something to that effect. 0005 includes
README.WARM, but I think there should be some place unified that
explains the whole thing.

+     * Most often and unless we are dealing with a pg-upgraded cluster, the
+     * root offset information should be cached. So there should not be too
+     * much overhead of fetching this information. Also, once a tuple is
+     * updated, the information will be copied to the new version. So it's not
+     * as if we're going to pay this price forever.

What if a tuple is updated -- presumably clearing the
HEAP_LATEST_TUPLE on the tuple at the end of the chain -- and then the
update aborts? Then we must be back to not having this information.

I will leave this question until I have grokked how this actually works.

One overall question about this patch series is how we feel about
using up this many bits. 0002 uses a bit from infomask, and 0005 uses
a bit from infomask2. I'm not sure if that's everything, and then I
think we're steeling some bits from the item pointers, too. While the
performance benefits of the patch sound pretty good based on the test
results so far, this is definitely the very last time we'll be able to
implement a feature that requires this many bits.

Yeah, this patch series uses a lot of bits. At some point we should
really add the "last full-scanned by version X" we discussed a long time
ago, and free the MOVED_IN / MOVED_OFF bits that have been unused for so
long. Sadly, once we add that, we need to wait one more release before
we can use the bits anyway.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, Mar 8, 2017 at 2:30 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Not really -- it's a bit slower actually in a synthetic case measuring
exactly the slowed-down case. See
/messages/by-id/CAD__OugK12ZqMWWjZiM-YyuD1y8JmMy6x9YEctNiF3rPp6hy0g@mail.gmail.com
I bet in normal cases it's unnoticeable. If WARM flies, then it's going
to provide a larger improvement than is lost to this.

Hmm, that test case isn't all that synthetic. It's just a single
column bulk update, which isn't anything all that crazy, and 5-10%
isn't nothing.

I'm kinda surprised it made that much difference, though.

--
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

Robert Haas wrote:

On Wed, Mar 8, 2017 at 2:30 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Not really -- it's a bit slower actually in a synthetic case measuring
exactly the slowed-down case. See
/messages/by-id/CAD__OugK12ZqMWWjZiM-YyuD1y8JmMy6x9YEctNiF3rPp6hy0g@mail.gmail.com
I bet in normal cases it's unnoticeable. If WARM flies, then it's going
to provide a larger improvement than is lost to this.

Hmm, that test case isn't all that synthetic. It's just a single
column bulk update, which isn't anything all that crazy,

The problem is that the update touches the second indexed column. With
the original code we would have stopped checking at that point, but with
the patched code we continue to verify all the other indexed columns for
changes.

Maybe we need more than one bitmapset to be given -- multiple ones for
for "any of these" checks (such as HOT, KEY and Identity) which can be
stopped as soon as one is found, and one for "all of these" (for WARM,
indirect indexes) which needs to be checked to completion.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

@@ -234,6 +236,21 @@ index_beginscan(Relation heapRelation,
scan->heapRelation = heapRelation;
scan->xs_snapshot = snapshot;

+	/*
+	 * If the index supports recheck, make sure that index tuple is saved
+	 * during index scans.
+	 *
+	 * XXX Ideally, we should look at all indexes on the table and check if
+	 * WARM is at all supported on the base table. If WARM is not supported
+	 * then we don't need to do any recheck. RelationGetIndexAttrBitmap() does
+	 * do that and sets rd_supportswarm after looking at all indexes. But we
+	 * don't know if the function was called earlier in the session when we're
+	 * here. We can't call it now because there exists a risk of causing
+	 * deadlock.
+	 */
+	if (indexRelation->rd_amroutine->amrecheck)
+		scan->xs_want_itup = true;
+
return scan;
}

I didn't like this comment very much. But it's not necessary: you have
already given relcache responsibility for setting rd_supportswarm. The
only problem seems to be that you set it in RelationGetIndexAttrBitmap
instead of RelationGetIndexList, but it's not clear to me why. I think
if the latter function is in charge, then we can trust the flag more
than the current situation. Let's set the value to false on relcache
entry build, for safety's sake.

I noticed that nbtinsert.c and nbtree.c have a bunch of new includes
that they don't actually need. Let's remove those. nbtutils.c does
need them because of btrecheck(). Speaking of which:

I have already commented about the executor involvement in btrecheck();
that doesn't seem good. I previously suggested to pass the EState down
from caller, but that's not a great idea either since you still need to
do the actual FormIndexDatum. I now think that a workable option would
be to compute the values/isnulls arrays so that btrecheck gets them
already computed. With that, this function would be no more of a
modularity violation that HeapSatisfiesHOTAndKey() itself.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

After looking at how index_fetch_heap and heap_hot_search_buffer
interact, I can't say I'm in love with the idea. I started thinking
that we should not have index_fetch_heap release the buffer lock only to
re-acquire it five lines later, so it should keep the buffer lock, do
the recheck and only release it afterwards (I realize that this means
there'd be need for two additional "else release buffer lock" branches);
but then this got me thinking that perhaps it would be better to have
another routine that does both call heap_hot_search_buffer and then call
recheck -- it occurs to me that what we're doing here is essentially
heap_warm_search_buffer.

Does that make sense?

Another thing is BuildIndexInfo being called over and over for each
recheck(). Surely we need to cache the indexinfo for each indexscan.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Tue, Mar 14, 2017 at 7:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

@@ -234,6 +236,21 @@ index_beginscan(Relation heapRelation,
scan->heapRelation = heapRelation;
scan->xs_snapshot = snapshot;

+     /*
+      * If the index supports recheck, make sure that index tuple is

saved

+      * during index scans.
+      *
+      * XXX Ideally, we should look at all indexes on the table and

check if

+ * WARM is at all supported on the base table. If WARM is not

supported

+ * then we don't need to do any recheck.

RelationGetIndexAttrBitmap() does

+ * do that and sets rd_supportswarm after looking at all indexes.

But we

+ * don't know if the function was called earlier in the session

when we're

+ * here. We can't call it now because there exists a risk of

causing

+      * deadlock.
+      */
+     if (indexRelation->rd_amroutine->amrecheck)
+             scan->xs_want_itup = true;
+
return scan;
}

I didn't like this comment very much. But it's not necessary: you have
already given relcache responsibility for setting rd_supportswarm. The
only problem seems to be that you set it in RelationGetIndexAttrBitmap
instead of RelationGetIndexList, but it's not clear to me why.

Hmm. I think you're right. Will fix that way and test.

I noticed that nbtinsert.c and nbtree.c have a bunch of new includes
that they don't actually need. Let's remove those. nbtutils.c does
need them because of btrecheck().

Right. It's probably a left over from the way I wrote the first version.
Will fix.

Speaking of which:

I have already commented about the executor involvement in btrecheck();
that doesn't seem good. I previously suggested to pass the EState down
from caller, but that's not a great idea either since you still need to
do the actual FormIndexDatum. I now think that a workable option would
be to compute the values/isnulls arrays so that btrecheck gets them
already computed.

I agree with your complaint about modularity violation. What I am unclear
is how passing values/isnulls array will fix that. The way code is
structured currently, recheck routines are called by index_fetch_heap(). So
if we try to compute values/isnulls in that function, we'll still need
access EState, which AFAIU will lead to similar violation. Or am I
mis-reading your idea?

I wonder if we should instead invent something similar to IndexRecheck(),
but instead of running ExecQual(), this new routine will compare the index
values by the given HeapTuple against given IndexTuple. ISTM that for this
to work we'll need to modify all callers of index_getnext() and teach them
to invoke the AM specific recheck method if xs_tuple_recheck flag is set to
true by index_getnext().

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Mar 14, 2017 at 5:17 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

After looking at how index_fetch_heap and heap_hot_search_buffer
interact, I can't say I'm in love with the idea. I started thinking
that we should not have index_fetch_heap release the buffer lock only to
re-acquire it five lines later, so it should keep the buffer lock, do
the recheck and only release it afterwards (I realize that this means
there'd be need for two additional "else release buffer lock" branches);

Yes, it makes sense.

but then this got me thinking that perhaps it would be better to have
another routine that does both call heap_hot_search_buffer and then call
recheck -- it occurs to me that what we're doing here is essentially
heap_warm_search_buffer.

Does that make sense?

We can do that, but it's not clear to me if that would be a huge
improvement. Also, I think we need to first decide on how to model the
recheck logic since that might affect this function significantly. For
example, if we decide to do recheck at a higher level then we will most
likely end up releasing and reacquiring the lock anyways.

Another thing is BuildIndexInfo being called over and over for each
recheck(). Surely we need to cache the indexinfo for each indexscan.

Good point. What should that place be though? Can we just cache them in the
relcache and maintain them along with the list of indexes? Looking at the
current callers, ExecOpenIndices() usually cache them in the ResultRelInfo,
which is sufficient because INSERT/UPDATE/DELETE code paths are the most
relevant paths where caching definitely helps. The only other place where
it may get called once per tuple is unique_key_recheck(), which is used for
deferred unique key tests and hence probably not very common.

BTW I wanted to share some more numbers from a recent performance test. I
thought it's important because the latest patch has fully functional chain
conversion code as well as all WAL-logging related pieces are in place
too. I ran these tests on a box borrowed from Tomas (thanks!). This has
64GB RAM and 350GB SSD with 1GB on-board RAM. I used the same test setup
that I used for the first test results reported on this thread i.e. a
modified pgbench_accounts table with additional columns and additional
indexes (one index on abalance so that every UPDATE is a potential WARM
update).

In a test where table + indexes exceeds RAM, running for 8hrs and
auto-vacuum parameters set such that we get 2-3 autovacuums on the table
during the test, we see WARM delivering more than 100% TPS as compared to
master. In this graph, I've plotted a moving average of TPS and the spikes
that we see coincides with the checkpoints (checkpoint_timeout is set to
20mins and max_wal_size large enough to avoid any xlog-based checkpoints).
The spikes are more prominent on WARM but I guess that's purely because it
delivers much higher TPS. I haven't shown here but I see WARM updates close
to 65-70% of the total updates. Also there is significant reduction in WAL
generated per txn.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Pavan Deolasee wrote:

On Tue, Mar 14, 2017 at 7:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

I have already commented about the executor involvement in btrecheck();
that doesn't seem good. I previously suggested to pass the EState down
from caller, but that's not a great idea either since you still need to
do the actual FormIndexDatum. I now think that a workable option would
be to compute the values/isnulls arrays so that btrecheck gets them
already computed.

I agree with your complaint about modularity violation. What I am unclear
is how passing values/isnulls array will fix that. The way code is
structured currently, recheck routines are called by index_fetch_heap(). So
if we try to compute values/isnulls in that function, we'll still need
access EState, which AFAIU will lead to similar violation. Or am I
mis-reading your idea?

You're right, it's still a problem. (Honestly, I think the whole idea
of trying to compute a fake index tuple starting from a just-read heap
tuple is a problem in itself; I just wonder if there's a way to do the
recheck that doesn't involve such a thing.)

I wonder if we should instead invent something similar to IndexRecheck(),
but instead of running ExecQual(), this new routine will compare the index
values by the given HeapTuple against given IndexTuple. ISTM that for this
to work we'll need to modify all callers of index_getnext() and teach them
to invoke the AM specific recheck method if xs_tuple_recheck flag is set to
true by index_getnext().

Yeah, grumble, that idea does sound intrusive, but perhaps it's
workable. What about bitmap indexscans? AFAICS we already have a
recheck there natively, so we only need to mark the page as lossy, which
we're already doing anyway.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Pavan Deolasee wrote:

BTW I wanted to share some more numbers from a recent performance test. I
thought it's important because the latest patch has fully functional chain
conversion code as well as all WAL-logging related pieces are in place
too. I ran these tests on a box borrowed from Tomas (thanks!). This has
64GB RAM and 350GB SSD with 1GB on-board RAM. I used the same test setup
that I used for the first test results reported on this thread i.e. a
modified pgbench_accounts table with additional columns and additional
indexes (one index on abalance so that every UPDATE is a potential WARM
update).

In a test where table + indexes exceeds RAM, running for 8hrs and
auto-vacuum parameters set such that we get 2-3 autovacuums on the table
during the test, we see WARM delivering more than 100% TPS as compared to
master. In this graph, I've plotted a moving average of TPS and the spikes
that we see coincides with the checkpoints (checkpoint_timeout is set to
20mins and max_wal_size large enough to avoid any xlog-based checkpoints).
The spikes are more prominent on WARM but I guess that's purely because it
delivers much higher TPS. I haven't shown here but I see WARM updates close
to 65-70% of the total updates. Also there is significant reduction in WAL
generated per txn.

Impressive results. Labels on axes would improve readability of the chart :-)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Tue, Mar 14, 2017 at 7:19 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Pavan Deolasee wrote:

BTW I wanted to share some more numbers from a recent performance test. I
thought it's important because the latest patch has fully functional

chain

conversion code as well as all WAL-logging related pieces are in place
too. I ran these tests on a box borrowed from Tomas (thanks!). This has
64GB RAM and 350GB SSD with 1GB on-board RAM. I used the same test setup
that I used for the first test results reported on this thread i.e. a
modified pgbench_accounts table with additional columns and additional
indexes (one index on abalance so that every UPDATE is a potential WARM
update).

In a test where table + indexes exceeds RAM, running for 8hrs and
auto-vacuum parameters set such that we get 2-3 autovacuums on the table
during the test, we see WARM delivering more than 100% TPS as compared to
master. In this graph, I've plotted a moving average of TPS and the

spikes

that we see coincides with the checkpoints (checkpoint_timeout is set to
20mins and max_wal_size large enough to avoid any xlog-based

checkpoints).

The spikes are more prominent on WARM but I guess that's purely because

it

delivers much higher TPS. I haven't shown here but I see WARM updates

close

to 65-70% of the total updates. Also there is significant reduction in

WAL

generated per txn.

Impressive results. Labels on axes would improve readability of the chart
:-)

Sorry about that. I was desperately searching for Undo button after hitting
"send" for the very same reason :-) Looks like I used gnuplot after a few
years.

Just to make it clear, the X-axis is duration of tests in seconds and
Y-axis is 450s moving average of TPS. BTW 450 is no magic figure. I
collected stats every 15s and took a moving average of last 30 samples.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Mar 14, 2017 at 12:19 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Impressive results.

Agreed.

It seems like an important invariant for WARM is that any duplicate
index values ought to have different TIDs (actually, it's a bit
stricter than that, since btrecheck() cares about simple binary
equality). ISTM that it would be fairly easy to modify amcheck such
that the "items in logical order" check, as well as the similar
"cross-page order" check (the one that detects transposed pages) also
check that this new WARM invariant holds. Obviously this would only
make sense on the leaf level of the index.

You wouldn't have to teach amcheck about the heap, because a TID that
points to the heap can only be duplicated within a B-Tree index
because of WARM. So, if we find that two adjacent tuples are equal,
check if the TIDs are equal. If they are also equal, check for strict
binary equality. If strict binary equality is indicated, throw an
error due to invariant failing.

IIUC, the design of WARM makes this simple enough to implement, and
cheap enough that the additional runtime overhead is well worthwhile.
You could just add this check to the existing checks without changing
the user-visible interface. It seems pretty complementary to what is
already there.

--
Peter Geoghegan

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

On Tue, Mar 14, 2017 at 7:16 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Pavan Deolasee wrote:

On Tue, Mar 14, 2017 at 7:17 AM, Alvaro Herrera <

alvherre@2ndquadrant.com>

wrote:

I have already commented about the executor involvement in btrecheck();
that doesn't seem good. I previously suggested to pass the EState down
from caller, but that's not a great idea either since you still need to
do the actual FormIndexDatum. I now think that a workable option would
be to compute the values/isnulls arrays so that btrecheck gets them
already computed.

I agree with your complaint about modularity violation. What I am unclear
is how passing values/isnulls array will fix that. The way code is
structured currently, recheck routines are called by index_fetch_heap().

So

if we try to compute values/isnulls in that function, we'll still need
access EState, which AFAIU will lead to similar violation. Or am I
mis-reading your idea?

You're right, it's still a problem. (Honestly, I think the whole idea
of trying to compute a fake index tuple starting from a just-read heap
tuple is a problem in itself;

Why do you think so?

I just wonder if there's a way to do the
recheck that doesn't involve such a thing.)

I couldn't find a better way without a lot of complex infrastructure. Even
though we now have ability to mark index pointers and we know that a given
pointer either points to the pre-WARM chain or post-WARM chain, this does
not solve the case when an index does not receive a new entry. In that
case, both pre-WARM and post-WARM tuples are reachable via the same old
index pointer. The only way we could deal with this is to mark index
pointers as "common", "pre-warm" and "post-warm". But that would require us
to update the old pointer's state from "common" to "pre-warm" for the index
whose keys are being updated. May be it's doable, but might be more complex
than the current approach.

I wonder if we should instead invent something similar to IndexRecheck(),
but instead of running ExecQual(), this new routine will compare the

index

values by the given HeapTuple against given IndexTuple. ISTM that for

this

to work we'll need to modify all callers of index_getnext() and teach

them

to invoke the AM specific recheck method if xs_tuple_recheck flag is set

to

true by index_getnext().

Yeah, grumble, that idea does sound intrusive, but perhaps it's
workable. What about bitmap indexscans? AFAICS we already have a
recheck there natively, so we only need to mark the page as lossy, which
we're already doing anyway.

Yeah, bitmap indexscans should be ok. We need recheck logic only to avoid
duplicate scans and since a TID can only occur once in the bitmap, there is
no risk for duplicate results.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Mar 15, 2017 at 3:44 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

I couldn't find a better way without a lot of complex infrastructure. Even
though we now have ability to mark index pointers and we know that a given
pointer either points to the pre-WARM chain or post-WARM chain, this does
not solve the case when an index does not receive a new entry. In that case,
both pre-WARM and post-WARM tuples are reachable via the same old index
pointer. The only way we could deal with this is to mark index pointers as
"common", "pre-warm" and "post-warm". But that would require us to update
the old pointer's state from "common" to "pre-warm" for the index whose keys
are being updated. May be it's doable, but might be more complex than the
current approach.

/me scratches head.

Aren't pre-warm and post-warm just (better) names for blue and red?

--
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

On Thu, Mar 16, 2017 at 12:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Mar 15, 2017 at 3:44 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

I couldn't find a better way without a lot of complex infrastructure.

Even

though we now have ability to mark index pointers and we know that a

given

pointer either points to the pre-WARM chain or post-WARM chain, this does
not solve the case when an index does not receive a new entry. In that

case,

both pre-WARM and post-WARM tuples are reachable via the same old index
pointer. The only way we could deal with this is to mark index pointers

as

"common", "pre-warm" and "post-warm". But that would require us to update
the old pointer's state from "common" to "pre-warm" for the index whose

keys

are being updated. May be it's doable, but might be more complex than the
current approach.

/me scratches head.

Aren't pre-warm and post-warm just (better) names for blue and red?

Yeah, sounds better. Just to make it clear, the current design sets the
following information:

HEAP_WARM_TUPLE - When a row gets WARM updated, both old and new versions
of the row are marked with HEAP_WARM_TUPLE flag. This allows us to remember
that a certain row was WARM-updated, even if the update later aborts and we
cleanup the new version and truncate the chain. All subsequent tuple
versions will carry this flag until a non-HOT updates happens, which breaks
the HOT chain.

HEAP_WARM_RED - After first WARM update, the new version of the tuple is
marked with this flag. This flag is also carried forward to all future HOT
updated tuples. So the only tuple that has HEAP_WARM_TUPLE but not
HEAP_WARM_RED is the old version before the WARM update. Also, all tuples
marked with HEAP_WARM_RED flag satisfies HOT property (i.e. all index key
columns share the same value). Similarly, all tuples NOT marked with
HEAP_WARM_RED also satisfy HOT property. I've so far called them Red and
Blue chains respectively.

In addition, in the current patch, the new index pointers resulted from
WARM updates are marked BTREE_INDEX_RED_POINTER/HASH_INDEX_RED_POINTER

I think per your suggestion we can change HEAP_WARM_RED to HEAP_WARM_TUPLE
and similarly rename the index pointers to BTREE/HASH_INDEX_WARM_POINTER
and replace HEAP_WARM_TUPLE with something like HEAP_WARM_UPDATED_TUPLE to
signify that this or some previous version of this chain was once
WARM-updated.

Does that sound ok? I can change the patch accordingly.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Mar 14, 2017 at 8:14 PM, Peter Geoghegan <pg@bowt.ie> wrote:

On Tue, Mar 14, 2017 at 12:19 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Impressive results.

Agreed.

Thanks. I repeated the same tests with slightly lower scale factor so that
most (but not all) data fits in memory. The results are kinda similar
(attached here). The spikes are still there and they correspond to the
checkpoint_timeout set for these tests.

It seems like an important invariant for WARM is that any duplicate
index values ought to have different TIDs (actually, it's a bit
stricter than that, since btrecheck() cares about simple binary
equality).

Yes. I think in the current code, indexes can never duplicate TIDs (at
least for btrees and hash). With WARM, indexes can have duplicate TIDs, but
iff index values differ. In addition there can only be one more duplicate
and one of them must be a Blue pointer (or a non-WARM pointer if we accept
the new nomenclature proposed a few mins back).

You wouldn't have to teach amcheck about the heap, because a TID that
points to the heap can only be duplicated within a B-Tree index
because of WARM. So, if we find that two adjacent tuples are equal,
check if the TIDs are equal. If they are also equal, check for strict
binary equality. If strict binary equality is indicated, throw an
error due to invariant failing.

Wouldn't this be much more expensive for non-unique indexes?

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Mar 19, 2017 at 3:05 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

On Thu, Mar 16, 2017 at 12:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Mar 15, 2017 at 3:44 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

I couldn't find a better way without a lot of complex infrastructure.
Even
though we now have ability to mark index pointers and we know that a
given
pointer either points to the pre-WARM chain or post-WARM chain, this
does
not solve the case when an index does not receive a new entry. In that
case,
both pre-WARM and post-WARM tuples are reachable via the same old index
pointer. The only way we could deal with this is to mark index pointers
as
"common", "pre-warm" and "post-warm". But that would require us to
update
the old pointer's state from "common" to "pre-warm" for the index whose
keys
are being updated. May be it's doable, but might be more complex than
the
current approach.

/me scratches head.

Aren't pre-warm and post-warm just (better) names for blue and red?

Yeah, sounds better.

My point here wasn't really about renaming, although I do think
renaming is something that should get done. My point was that you
were saying we need to mark index pointers as common, pre-warm, and
post-warm. But you're pretty much already doing that, I think. I
guess you don't have "common", but you do have "pre-warm" and
"post-warm".

--
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

On Mon, Mar 20, 2017 at 8:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Mar 19, 2017 at 3:05 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

On Thu, Mar 16, 2017 at 12:53 PM, Robert Haas <robertmhaas@gmail.com>

wrote:

/me scratches head.

Aren't pre-warm and post-warm just (better) names for blue and red?

Yeah, sounds better.

My point here wasn't really about renaming, although I do think
renaming is something that should get done. My point was that you
were saying we need to mark index pointers as common, pre-warm, and
post-warm. But you're pretty much already doing that, I think. I
guess you don't have "common", but you do have "pre-warm" and
"post-warm".

Ah, I mis-read that. Strictly speaking, we already have common (blue) and
post-warm (red), and I just finished renaming them to CLEAR (of WARM bit)
and WARM. May be it's still not the best name, but I think it looks better
than before.

But the larger point is that we don't have an easy to know if an index
pointer which was inserted with the original heap tuple (i.e. pre-WARM
update) should only return pre-WARM tuples or should it also return
post-WARM tuples. Right now we make that decision by looking at the
index-keys and discard the pointer whose index-key does not match the ones
created from heap-keys. If we need to change that then at every WARM
update, we will have to go back to the original pointer and change it's
state to pre-warm. That looks more invasive and requires additional index
management.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Mar 15, 2017 at 12:46 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Pavan Deolasee wrote:

On Tue, Mar 14, 2017 at 7:17 AM, Alvaro Herrera <

alvherre@2ndquadrant.com>

wrote:

I have already commented about the executor involvement in btrecheck();
that doesn't seem good. I previously suggested to pass the EState down
from caller, but that's not a great idea either since you still need to
do the actual FormIndexDatum. I now think that a workable option would
be to compute the values/isnulls arrays so that btrecheck gets them
already computed.

I agree with your complaint about modularity violation. What I am unclear
is how passing values/isnulls array will fix that. The way code is
structured currently, recheck routines are called by index_fetch_heap().

So

if we try to compute values/isnulls in that function, we'll still need
access EState, which AFAIU will lead to similar violation. Or am I
mis-reading your idea?

You're right, it's still a problem.

BTW I realised that we don't really need those executor bits in recheck
routines. We don't support WARM when attributes in index expressions are
modified. So we really don't need to do any comparison for those
attributes. I've written a separate form of FormIndexDatum() which will
only return basic index attributes and comparing them should be enough.
Will share rebased and updated patch soon.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Mar 14, 2017 at 7:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

@@ -234,6 +236,21 @@ index_beginscan(Relation heapRelation,
scan->heapRelation = heapRelation;
scan->xs_snapshot = snapshot;

+     /*
+      * If the index supports recheck, make sure that index tuple is

saved

+      * during index scans.
+      *
+      * XXX Ideally, we should look at all indexes on the table and

check if

+ * WARM is at all supported on the base table. If WARM is not

supported

+ * then we don't need to do any recheck.

RelationGetIndexAttrBitmap() does

+ * do that and sets rd_supportswarm after looking at all indexes.

But we

+ * don't know if the function was called earlier in the session

when we're

+ * here. We can't call it now because there exists a risk of

causing

+      * deadlock.
+      */
+     if (indexRelation->rd_amroutine->amrecheck)
+             scan->xs_want_itup = true;
+
return scan;
}

I didn't like this comment very much. But it's not necessary: you have
already given relcache responsibility for setting rd_supportswarm. The
only problem seems to be that you set it in RelationGetIndexAttrBitmap
instead of RelationGetIndexList, but it's not clear to me why. I think
if the latter function is in charge, then we can trust the flag more
than the current situation.

I looked at this today. AFAICS we don't have access to rd_amroutine in
RelationGetIndexList since we don't actually call index_open() in that
function. Would it be safe to do that? I'll give it a shot, but thought of
asking here first.

Thanks,
Pavan

Pavan Deolasee wrote:

On Tue, Mar 14, 2017 at 7:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

I didn't like this comment very much. But it's not necessary: you have
already given relcache responsibility for setting rd_supportswarm. The
only problem seems to be that you set it in RelationGetIndexAttrBitmap
instead of RelationGetIndexList, but it's not clear to me why. I think
if the latter function is in charge, then we can trust the flag more
than the current situation.

I looked at this today. AFAICS we don't have access to rd_amroutine in
RelationGetIndexList since we don't actually call index_open() in that
function. Would it be safe to do that? I'll give it a shot, but thought of
asking here first.

Ah, you're right, we only have the pg_index tuple for the index, not the
pg_am one. I think one pg_am cache lookup isn't really all that
terrible (though we should ensure that there's no circularity problem in
doing that), but I doubt that going to the trouble of invoking the
amhandler just to figure out if it supports WARM is acceptable.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Sun, Mar 19, 2017 at 12:15 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

It seems like an important invariant for WARM is that any duplicate
index values ought to have different TIDs (actually, it's a bit
stricter than that, since btrecheck() cares about simple binary
equality).

Yes. I think in the current code, indexes can never duplicate TIDs (at least
for btrees and hash). With WARM, indexes can have duplicate TIDs, but iff
index values differ. In addition there can only be one more duplicate and
one of them must be a Blue pointer (or a non-WARM pointer if we accept the
new nomenclature proposed a few mins back).

It looks like those additional Red/Blue details are available right
from the IndexTuple, which makes the check a good fit for amcheck (no
need to bring the heap into it).

You wouldn't have to teach amcheck about the heap, because a TID that
points to the heap can only be duplicated within a B-Tree index
because of WARM. So, if we find that two adjacent tuples are equal,
check if the TIDs are equal. If they are also equal, check for strict
binary equality. If strict binary equality is indicated, throw an
error due to invariant failing.

Wouldn't this be much more expensive for non-unique indexes?

Only in the worst case, where there are many many duplicates, and only
if you insisted on being completely comprehensive, rather than merely
very comprehensive. That is, you can store the duplicate TIDs in local
memory up to a quasi-arbitrary budget, since you do have to make sure
that any local buffer cannot grow in an unbounded fashion. Certainly,
if you stored 10,000 TIDs, there is always going to be a theoretical
case where that wasn't enough. But you can always say something like
that. We are defending against Murphy here, not Machiavelli.

You're going to have to qsort() a particular value's duplicate TIDs
once you encounter a distinct value, and therefore need to evaluate
the invariant. That's not a big deal, because sorting less than 1,000
items is generally very fast. It's well worth it. I'd probably choose
a generic budget for storing TIDs in local memory, and throw out half
of the TIDs when that budget is exceeded.

I see no difficulty with race conditions when you have only an
AccessShareLock on target. Concurrent page splits won't hurt, because
you reliably skip over those by always moving right. I'm pretty sure
that VACUUM killing IndexTuples that you've already stored with the
intention of sorting later is also not a complicating factor, since
you know that the heap TIDs that are WARM root pointers are not going
to be recycled in the lifetime of the amcheck query such that you get
a false positive.

A WARM check seems like a neat adjunct to what amcheck does already.
It seems like a really good idea for WARM to buy into this kind of
verification. It is, at worst, cheap insurance.

--
Peter Geoghegan

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

On Fri, Mar 10, 2017 at 11:37 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Robert Haas wrote:

On Wed, Mar 8, 2017 at 2:30 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Not really -- it's a bit slower actually in a synthetic case measuring
exactly the slowed-down case. See
/messages/by-id/CAD__OugK12ZqMWWjZiM-YyuD1y8JmMy6x9YEctNiF3rPp6hy0g@mail.gmail.com
I bet in normal cases it's unnoticeable. If WARM flies, then it's going
to provide a larger improvement than is lost to this.

Hmm, that test case isn't all that synthetic. It's just a single
column bulk update, which isn't anything all that crazy,

The problem is that the update touches the second indexed column. With
the original code we would have stopped checking at that point, but with
the patched code we continue to verify all the other indexed columns for
changes.

Maybe we need more than one bitmapset to be given -- multiple ones for
for "any of these" checks (such as HOT, KEY and Identity) which can be
stopped as soon as one is found, and one for "all of these" (for WARM,
indirect indexes) which needs to be checked to completion.

How will that help to mitigate the regression? I think what might
help here is if we fetch the required columns for WARM only when we
know hot_update is false.

--
With Regards,
Amit Kapila.
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

On Thu, Mar 9, 2017 at 8:43 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Mar 8, 2017 at 2:30 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Not really -- it's a bit slower actually in a synthetic case measuring
exactly the slowed-down case. See
/messages/by-id/CAD__OugK12ZqMWWjZiM-YyuD1y8JmMy6x9YEctNiF3rPp6hy0g@mail.gmail.com
I bet in normal cases it's unnoticeable. If WARM flies, then it's going
to provide a larger improvement than is lost to this.

Hmm, that test case isn't all that synthetic. It's just a single
column bulk update, which isn't anything all that crazy, and 5-10%
isn't nothing.

I'm kinda surprised it made that much difference, though.

I think it is because heap_getattr() is not that cheap. We have
noticed the similar problem during development of scan key push down
work [1]https://commitfest.postgresql.org/12/850/.

[1]: https://commitfest.postgresql.org/12/850/

--
With Regards,
Amit Kapila.
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

On Tue, Mar 21, 2017 at 6:56 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Hmm, that test case isn't all that synthetic. It's just a single
column bulk update, which isn't anything all that crazy, and 5-10%
isn't nothing.

I'm kinda surprised it made that much difference, though.

I think it is because heap_getattr() is not that cheap. We have
noticed the similar problem during development of scan key push down
work [1].

Yeah. So what's the deal with this? Is somebody working on figuring
out a different approach that would reduce this overhead? Are we
going to defer WARM to v11? Or is the intent to just ignore the 5-10%
slowdown on a single column update and commit everything anyway? (A
strong -1 on that course of action from me.)

--
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

On Tue, Mar 21, 2017 at 5:34 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Mar 21, 2017 at 6:56 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

Hmm, that test case isn't all that synthetic. It's just a single
column bulk update, which isn't anything all that crazy, and 5-10%
isn't nothing.

I'm kinda surprised it made that much difference, though.

I think it is because heap_getattr() is not that cheap. We have
noticed the similar problem during development of scan key push down
work [1].

Yeah. So what's the deal with this? Is somebody working on figuring
out a different approach that would reduce this overhead? Are we
going to defer WARM to v11? Or is the intent to just ignore the 5-10%
slowdown on a single column update and commit everything anyway?

I think I should clarify something. The test case does a single column
update, but it also has columns which are very wide, has an index on many
columns (and it updates a column early in the list). In addition, in the
test Mithun updated all 10million rows of the table in a single
transaction, used UNLOGGED table and fsync was turned off.

TBH I see many artificial scenarios here. It will be very useful if he can
rerun the query with some of these restrictions lifted. I'm all for
addressing whatever we can, but I am not sure if this test demonstrates a
real world usage.

Having said that, may be if we can do a few things to reduce the overhead.

- Check if the page has enough free space to perform a HOT/WARM update. If
not, don't look for all index keys.
- Pass bitmaps separately for each index and bail out early if we conclude
neither HOT nor WARM is possible. In this case since there is just one
index and as soon as we check the second column we know neither HOT nor
WARM is possible, we will return early. It might complicate the API a lot,
but I can give it a shot if that's what is needed to make progress.

Any other ideas?

Thanks,
Pavan
--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Mar 14, 2017 at 10:47 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

After looking at how index_fetch_heap and heap_hot_search_buffer
interact, I can't say I'm in love with the idea. I started thinking
that we should not have index_fetch_heap release the buffer lock only to
re-acquire it five lines later, so it should keep the buffer lock, do
the recheck and only release it afterwards (I realize that this means
there'd be need for two additional "else release buffer lock" branches);
but then this got me thinking that perhaps it would be better to have
another routine that does both call heap_hot_search_buffer and then call
recheck -- it occurs to me that what we're doing here is essentially
heap_warm_search_buffer.

Does that make sense?

Another thing is BuildIndexInfo being called over and over for each
recheck(). Surely we need to cache the indexinfo for each indexscan.

Please find attached rebased patches. There are a few changes in this
version, so let me mention them here instead of trying to reply in-line to
various points on various emails:

1. The patch now has support for hash redo recovery since that was added to
the master (it might be broken since a bug was reported in the original
code itself)

2. Based on Robert's comments and my discussion with him in person, I
removed the Blue/Red naming and instead now using CLEAR and WARM to
identify the parts of the chain and the index pointers. This also resulted
in changes to the way heap tuple header bits are named. So
HEAP_WARM_UPDATED is now used to mark the old tuple which gets WARM updated
and the same flag is copied to all subsequent versions of the tuple, until
a non-HOT updates happens. The new version and all subsequent versions are
marked with HEAP_WARM_TUPLE flag (in the earlier versions this was used for
marking old and the new versions. This might cause confusion, but looks a
more accurate naming to me.

3. IndexInfo is now cached inside IndexScanDescData, which should address
your comment above.

4. I realised that we don't really need to ever compare expression
attributes in the index since WARM is never used when one of those columns
is updated. Hence I've now created a new version of FormIndexDatum which
only returns plain attributes and hence recheck routine does not need
access to any executor stuff.

5. We don't release the lock of the buffer if we are going to apply
recheck. This should address part of the your comment. I haven't though put
them inside a single wrapper function because there is just one caller to
amrecheck function and after this change, it looked ok. But if you don't
still like, I'll make that change.

6. Unnecessary header files included at various places have been removed.

7. Some comments have been updated and rewritten. Hopefully they look
better than before now.

8. I merged the main WARM patch and the chain conversion code in a single
patch since I don't think we will apply them separately. But if it helps
with review, let me know and I can split that again.

9. I realised that we don't really need xs_tuple_recheck in the scan
descriptor and hence removed that and used a stack variable to get that
info.

10. Accidentally WARM was disabled on the system relations during one of
the earlier rebases. So restored that back and made a slight change to
regression expected output.

All tests pass with the patch set. I am now writing TAP tests for WARM and
will submit that separately. Per your suggestion, I am first turning the
stress tests I'd used earlier to use TAP tests and then add more tests,
especially around recovery and index addition/deletion.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Mar 21, 2017 at 8:41 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

Yeah. So what's the deal with this? Is somebody working on figuring
out a different approach that would reduce this overhead? Are we
going to defer WARM to v11? Or is the intent to just ignore the 5-10%
slowdown on a single column update and commit everything anyway?

I think I should clarify something. The test case does a single column
update, but it also has columns which are very wide, has an index on many
columns (and it updates a column early in the list). In addition, in the
test Mithun updated all 10million rows of the table in a single transaction,
used UNLOGGED table and fsync was turned off.

TBH I see many artificial scenarios here. It will be very useful if he can
rerun the query with some of these restrictions lifted. I'm all for
addressing whatever we can, but I am not sure if this test demonstrates a
real world usage.

That's a very fair point, but if these patches - or some of them - are
going to get committed then these things need to get discussed. Let's
not just have nothing-nothing-nothing giant unagreed code drop.

I think that very wide columns and highly indexed tables are not
particularly unrealistic, nor do I think updating all the rows is
particularly unrealistic. Sure, it's not everything, but it's
something. Now, I would agree that all of that PLUS unlogged tables
with fsync=off is not too realistic. What kind of regression would we
observe if we eliminated those last two variables?

Having said that, may be if we can do a few things to reduce the overhead.

- Check if the page has enough free space to perform a HOT/WARM update. If
not, don't look for all index keys.
- Pass bitmaps separately for each index and bail out early if we conclude
neither HOT nor WARM is possible. In this case since there is just one index
and as soon as we check the second column we know neither HOT nor WARM is
possible, we will return early. It might complicate the API a lot, but I can
give it a shot if that's what is needed to make progress.

I think that whether the code ends up getting contorted is an
important consideration here. For example, if the first of the things
you mention can be done without making the code ugly, then I think
that would be worth doing; it's likely to help fairly often in
real-world cases. The problem with making the code contorted and
ugly, as you say that the second idea would require, is that it can
easily mask bugs.

--
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

Amit Kapila wrote:

I think it is because heap_getattr() is not that cheap. We have
noticed the similar problem during development of scan key push down
work [1].

One possibility to reduce the cost of that is to use whole tuple deform
instead of repeated individual heap_getattr() calls. Since we don't
actually need *all* attrs, we can create a version of heap_deform_tuple
that takes an attribute number as argument and decodes up to that point.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Tue, Mar 21, 2017 at 6:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Mar 21, 2017 at 8:41 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:

Yeah. So what's the deal with this? Is somebody working on figuring
out a different approach that would reduce this overhead? Are we
going to defer WARM to v11? Or is the intent to just ignore the 5-10%
slowdown on a single column update and commit everything anyway?

I think I should clarify something. The test case does a single column
update, but it also has columns which are very wide, has an index on many
columns (and it updates a column early in the list). In addition, in the
test Mithun updated all 10million rows of the table in a single transaction,
used UNLOGGED table and fsync was turned off.

TBH I see many artificial scenarios here. It will be very useful if he can
rerun the query with some of these restrictions lifted. I'm all for
addressing whatever we can, but I am not sure if this test demonstrates a
real world usage.

That's a very fair point, but if these patches - or some of them - are
going to get committed then these things need to get discussed. Let's
not just have nothing-nothing-nothing giant unagreed code drop.

I think that very wide columns and highly indexed tables are not
particularly unrealistic, nor do I think updating all the rows is
particularly unrealistic. Sure, it's not everything, but it's
something. Now, I would agree that all of that PLUS unlogged tables
with fsync=off is not too realistic. What kind of regression would we
observe if we eliminated those last two variables?

Sure, we can try that. I think we need to try it with
synchronous_commit = off, otherwise, WAL writes completely overshadows
everything.

--
With Regards,
Amit Kapila.
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

On Tue, Mar 21, 2017 at 10:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I think that very wide columns and highly indexed tables are not
particularly unrealistic, nor do I think updating all the rows is
particularly unrealistic. Sure, it's not everything, but it's
something. Now, I would agree that all of that PLUS unlogged tables
with fsync=off is not too realistic. What kind of regression would we
observe if we eliminated those last two variables?

Sure, we can try that. I think we need to try it with
synchronous_commit = off, otherwise, WAL writes completely overshadows
everything.

synchronous_commit = off is a much more realistic scenario than fsync = off.

--
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

On Tue, Mar 21, 2017 at 6:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I think that very wide columns and highly indexed tables are not
particularly unrealistic, nor do I think updating all the rows is
particularly unrealistic.

Ok. But those who update 10M rows in a single transaction, would they
really notice 5-10% variation? I think it probably makes sense to run those
updates in smaller transactions and see if the regression is still visible
(otherwise tweaking synchronous_commit is mute anyways).

Sure, it's not everything, but it's
something. Now, I would agree that all of that PLUS unlogged tables
with fsync=off is not too realistic. What kind of regression would we
observe if we eliminated those last two variables?

Hard to say. I didn't find any regression on the machines available to me
even with the original test case that I used, which was pretty bad case to
start with (sure, Mithun tweaked it further to create even worse scenario).
May be the kind of machines he has access to, it might show up even with
those changes.