Thread: Patch: Write Amplification Reduction Method (WARM)

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 +

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 +

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 +

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

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

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

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

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

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

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

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

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,
thesecond pull_varattnos() call.
 

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

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
considersthe 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_TUPLEset; but tuples upgraded from earlier versions do not.  For those, we determine whether a tuple is
latestby 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 doctid = 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

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

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

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

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,
https://postgr.es/m/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

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

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

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

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

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

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

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

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.

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

* 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

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

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

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

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,
CatalogIndexStateindstate);
 

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

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

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

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

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 +

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 +

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

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 +

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

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 +

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

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 +

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

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 +

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

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

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

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

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 +

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 +

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

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

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

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

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

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
https://www.postgresql.org/message-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

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
> https://www.postgresql.org/message-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

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
> > https://www.postgresql.org/message-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

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

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

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

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

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

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

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

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

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

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
>> > https://www.postgresql.org/message-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

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
>> https://www.postgresql.org/message-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].

[1] - https://commitfest.postgresql.org/12/850/

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

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

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

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

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

Robert Haas wrote:
> On Tue, Mar 21, 2017 at 10:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

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

Sure, synchronous_commit=off is a reasonable case.  But I say if we lose
a few % on the case where you update only the first indexed of a large
number of very wide columns all indexed, and this is only noticeable if
you don't write WAL and only if you update all the rows in the table,
then I don't see much reason for concern.

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

On Tue, Mar 21, 2017 at 10:21 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> On Tue, Mar 21, 2017 at 10:01 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
>> > 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.
>
> Sure, synchronous_commit=off is a reasonable case.  But I say if we lose
> a few % on the case where you update only the first indexed of a large
> number of very wide columns all indexed, and this is only noticeable if
> you don't write WAL and only if you update all the rows in the table,
> then I don't see much reason for concern.

If the WAL writing hides the loss, then I agree that's not a big
concern.  But if the loss is still visible even when WAL is written,
then I'm not so sure.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2017-03-21 08:04:11 -0400, Robert Haas 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?

I think one reasonable thing would be to use slots here, and use
slot_getsomeattrs(), with a pre-computed offset, for doing the
deforming.  Given that more than one place run into the issue with
deforming cost via heap_*, that seems like something we're going to have
to do.  Additionally the patches I had for JITed deforming all
integrated at the slot layer, so it'd be a good thing from that angle as
well.

Deforming all columns at once would also a boon for the accompanying
index_getattr calls.


Greetings,

Andres Freund

On 2017-03-21 19:49:07 +0530, Pavan Deolasee wrote:
> 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?

Yes. It's very common in ETL, and that's quite performance sensitive.

Greetings,

Andres Freund

On Tue, Mar 21, 2017 at 09:25:49AM -0400, Robert Haas wrote:
> On Tue, Mar 21, 2017 at 8:41 AM, Pavan Deolasee
> > 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.

First, let me say I love this feature for PG 10, along with
multi-variate statistics.

However, not to be a bummer on this, but the persistent question I have
is whether we are locking ourselves into a feature that can only do
_one_ index-change per WARM chain before a lazy vacuum is required.  Are
we ever going to figure out how to do more changes per WARM chain in the
future, and is our use of so many bits for this feature going to
restrict our ability to do that in the future.

I know we have talked about it, but not recently, and if everyone else
is fine with it, I am too, but I have to ask these questions.

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

On Tue, Mar 21, 2017 at 12:49 PM, Bruce Momjian <bruce@momjian.us> wrote:
> On Tue, Mar 21, 2017 at 09:25:49AM -0400, Robert Haas wrote:
>> On Tue, Mar 21, 2017 at 8:41 AM, Pavan Deolasee
>> > 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.
>
> First, let me say I love this feature for PG 10, along with
> multi-variate statistics.
>
> However, not to be a bummer on this, but the persistent question I have
> is whether we are locking ourselves into a feature that can only do
> _one_ index-change per WARM chain before a lazy vacuum is required.  Are
> we ever going to figure out how to do more changes per WARM chain in the
> future, and is our use of so many bits for this feature going to
> restrict our ability to do that in the future.
>
> I know we have talked about it, but not recently, and if everyone else
> is fine with it, I am too, but I have to ask these questions.

I think that's a good question.  I previously expressed similar
concerns.  On the one hand, it's hard to ignore the fact that, in the
cases where this wins, it already buys us a lot of performance
improvement.  On the other hand, as you say (and as I said), it eats
up a lot of bits, and that limits what we can do in the future.  On
the one hand, there is a saying that a bird in the hand is worth two
in the bush.  On the other hand, there is also a saying that one
should not paint oneself into the corner.

I'm not sure we've had any really substantive discussion of these
issues.  Pavan's response to my previous comments was basically "well,
I think it's worth it", which is entirely reasonable, because he
presumably wouldn't have written the patch that way if he thought it
sucked.  But it might not be the only opinion.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 21, 2017 at 10:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> I think that's a good question.  I previously expressed similar
> concerns.  On the one hand, it's hard to ignore the fact that, in the
> cases where this wins, it already buys us a lot of performance
> improvement.  On the other hand, as you say (and as I said), it eats
> up a lot of bits, and that limits what we can do in the future.  On
> the one hand, there is a saying that a bird in the hand is worth two
> in the bush.  On the other hand, there is also a saying that one
> should not paint oneself into the corner.

Are we really saying that there can be no incompatible change to the
on-disk representation for the rest of eternity? I can see why that's
something to avoid indefinitely, but I wouldn't like to rule it out.

-- 
Peter Geoghegan

On Tue, Mar 21, 2017 at 1:08 PM, Peter Geoghegan <pg@bowt.ie> wrote:
> On Tue, Mar 21, 2017 at 10:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I think that's a good question.  I previously expressed similar
>> concerns.  On the one hand, it's hard to ignore the fact that, in the
>> cases where this wins, it already buys us a lot of performance
>> improvement.  On the other hand, as you say (and as I said), it eats
>> up a lot of bits, and that limits what we can do in the future.  On
>> the one hand, there is a saying that a bird in the hand is worth two
>> in the bush.  On the other hand, there is also a saying that one
>> should not paint oneself into the corner.
>
> Are we really saying that there can be no incompatible change to the
> on-disk representation for the rest of eternity? I can see why that's
> something to avoid indefinitely, but I wouldn't like to rule it out.

Well, I don't want to rule it out either, but if we do a release to
which you can't pg_upgrade, it's going to be really painful for a lot
of users.  Many users can't realistically upgrade using pg_dump, ever.
So they'll be stuck on the release before the one that breaks
compatibility for a very long time.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 21, 2017 at 01:04:14PM -0400, Robert Haas wrote:
> > I know we have talked about it, but not recently, and if everyone else
> > is fine with it, I am too, but I have to ask these questions.
> 
> I think that's a good question.  I previously expressed similar
> concerns.  On the one hand, it's hard to ignore the fact that, in the
> cases where this wins, it already buys us a lot of performance
> improvement.  On the other hand, as you say (and as I said), it eats
> up a lot of bits, and that limits what we can do in the future.  On
> the one hand, there is a saying that a bird in the hand is worth two
> in the bush.  On the other hand, there is also a saying that one
> should not paint oneself into the corner.
> 
> I'm not sure we've had any really substantive discussion of these
> issues.  Pavan's response to my previous comments was basically "well,
> I think it's worth it", which is entirely reasonable, because he
> presumably wouldn't have written the patch that way if he thought it
> sucked.  But it might not be the only opinion.

Early in the discussion we talked about allowing multiple changes per
WARM chain if they all changed the same index and were in the same
direction so there were no duplicates, but it was complicated.  There
was also discussion about checking the index during INSERT/UPDATE to see
if there was a duplicate.  However, those ideas never led to further
discussion.

I know the current patch yields good results, but only on a narrow test
case, so I am not ready to just stop asking questions based the opinion
of the author or test results alone.

If someone came to me and said, "We have thought about allowing more
than one index change per WARM chain, and if we can ever do it, it will
probably be done this way, and we have the bits for it," I would be more
comfortable.

One interesting side-issue is that indirect indexes have a similar
problem with duplicate index entries, and there is no plan on how to fix
that either.  I guess I just don't feel we have explored the
duplicate-index-entry problem enough for me to be comfortable.

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

On Tue, Mar 21, 2017 at 01:14:00PM -0400, Robert Haas wrote:
> On Tue, Mar 21, 2017 at 1:08 PM, Peter Geoghegan <pg@bowt.ie> wrote:
> > On Tue, Mar 21, 2017 at 10:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> >> I think that's a good question.  I previously expressed similar
> >> concerns.  On the one hand, it's hard to ignore the fact that, in the
> >> cases where this wins, it already buys us a lot of performance
> >> improvement.  On the other hand, as you say (and as I said), it eats
> >> up a lot of bits, and that limits what we can do in the future.  On
> >> the one hand, there is a saying that a bird in the hand is worth two
> >> in the bush.  On the other hand, there is also a saying that one
> >> should not paint oneself into the corner.
> >
> > Are we really saying that there can be no incompatible change to the
> > on-disk representation for the rest of eternity? I can see why that's
> > something to avoid indefinitely, but I wouldn't like to rule it out.
> 
> Well, I don't want to rule it out either, but if we do a release to
> which you can't pg_upgrade, it's going to be really painful for a lot
> of users.  Many users can't realistically upgrade using pg_dump, ever.
> So they'll be stuck on the release before the one that breaks
> compatibility for a very long time.

Right.  If we weren't setting tuple and tid bits we could imrpove it
easily in PG 11, but if we use them for a single-change WARM chain for
PG 10, we might need bits that are not available to improve it later.

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

On 21/03/17 18:14, Robert Haas wrote:
> On Tue, Mar 21, 2017 at 1:08 PM, Peter Geoghegan <pg@bowt.ie> wrote:
>> On Tue, Mar 21, 2017 at 10:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> I think that's a good question.  I previously expressed similar
>>> concerns.  On the one hand, it's hard to ignore the fact that, in the
>>> cases where this wins, it already buys us a lot of performance
>>> improvement.  On the other hand, as you say (and as I said), it eats
>>> up a lot of bits, and that limits what we can do in the future.  On
>>> the one hand, there is a saying that a bird in the hand is worth two
>>> in the bush.  On the other hand, there is also a saying that one
>>> should not paint oneself into the corner.
>>
>> Are we really saying that there can be no incompatible change to the
>> on-disk representation for the rest of eternity? I can see why that's
>> something to avoid indefinitely, but I wouldn't like to rule it out.
> 
> Well, I don't want to rule it out either, but if we do a release to
> which you can't pg_upgrade, it's going to be really painful for a lot
> of users.  Many users can't realistically upgrade using pg_dump, ever.
> So they'll be stuck on the release before the one that breaks
> compatibility for a very long time.
> 

This is why I like the idea of pluggable storage, if we ever get that it
would buy us ability to implement completely different heap format
without breaking pg_upgrade.

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

On 21/03/17 18:19, Bruce Momjian wrote:
> On Tue, Mar 21, 2017 at 01:14:00PM -0400, Robert Haas wrote:
>> On Tue, Mar 21, 2017 at 1:08 PM, Peter Geoghegan <pg@bowt.ie> wrote:
>>> On Tue, Mar 21, 2017 at 10:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> I think that's a good question.  I previously expressed similar
>>>> concerns.  On the one hand, it's hard to ignore the fact that, in the
>>>> cases where this wins, it already buys us a lot of performance
>>>> improvement.  On the other hand, as you say (and as I said), it eats
>>>> up a lot of bits, and that limits what we can do in the future.  On
>>>> the one hand, there is a saying that a bird in the hand is worth two
>>>> in the bush.  On the other hand, there is also a saying that one
>>>> should not paint oneself into the corner.
>>>
>>> Are we really saying that there can be no incompatible change to the
>>> on-disk representation for the rest of eternity? I can see why that's
>>> something to avoid indefinitely, but I wouldn't like to rule it out.
>>
>> Well, I don't want to rule it out either, but if we do a release to
>> which you can't pg_upgrade, it's going to be really painful for a lot
>> of users.  Many users can't realistically upgrade using pg_dump, ever.
>> So they'll be stuck on the release before the one that breaks
>> compatibility for a very long time.
> 
> Right.  If we weren't setting tuple and tid bits we could imrpove it
> easily in PG 11, but if we use them for a single-change WARM chain for
> PG 10, we might need bits that are not available to improve it later.
> 

I thought there is still couple of bits available.

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

On Tue, Mar 21, 2017 at 2:03 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> This is why I like the idea of pluggable storage, if we ever get that it
> would buy us ability to implement completely different heap format
> without breaking pg_upgrade.

You probably won't be surprised to hear that I agree.  :-)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 21, 2017 at 07:05:15PM +0100, Petr Jelinek wrote:
> >> Well, I don't want to rule it out either, but if we do a release to
> >> which you can't pg_upgrade, it's going to be really painful for a lot
> >> of users.  Many users can't realistically upgrade using pg_dump, ever.
> >> So they'll be stuck on the release before the one that breaks
> >> compatibility for a very long time.
> > 
> > Right.  If we weren't setting tuple and tid bits we could improve it
> > easily in PG 11, but if we use them for a single-change WARM chain for
> > PG 10, we might need bits that are not available to improve it later.
> > 
> 
> I thought there is still couple of bits available.

Yes, there are.  The issue is that we don't know how we would improve it
so we don't know how many bits we need, and my concern is that we
haven't discussed the improvement ideas enough to know we have done the
best we can for PG 10.

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

On Tue, Mar 21, 2017 at 11:45:09PM +0530, Pavan Deolasee wrote:
>     Early in the discussion we talked about allowing multiple changes per
>     WARM chain if they all changed the same index and were in the same
>     direction so there were no duplicates, but it was complicated.  There
>     was also discussion about checking the index during INSERT/UPDATE to see
>     if there was a duplicate.  However, those ideas never led to further
>     discussion.
> 
> 
> Well, once I started thinking about how to do vacuum etc, I realised that any
> mechanism which allows unlimited (even handful) updates per chain is going to
> be very complex and error prone. But if someone has ideas to do that, I am
> open. I must say though, it will make an already complex problem even more
> complex.

Yes, that is where we got stuck.  Have enough people studied the issue
to know that there are no simple answers?

>     I know the current patch yields good results, but only on a narrow test
>     case,
> 
> 
> Hmm. I am kinda surprised you say that because I never thought it was a narrow
> test case that we are targeting here. But may be I'm wrong.

Well, it is really a question of how often you want to do a second WARM
update (not possible) vs. the frequency of lazy vacuum.  I assumed that
would be a 100X or 10kX difference, but I am not sure myself either.  My
initial guess was that only allowing a single WARM update between lazy
vacuums would show no improvementin in real-world workloads, but maybe I
am wrong.

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

On Tue, Mar 21, 2017 at 11:54:25PM +0530, Pavan Deolasee wrote:
> We can also save HEAP_WARM_UPDATED flag since this is required only for
> abort-handling case. We can find a way to push that information down to the old
> tuple if UPDATE aborts and we detect the broken chain. Again, not fully thought
> through, but doable. Of course, we will have to carefully evaluate all code
> paths and make sure that we don't lose that information ever.
> 
> If the consumption of bits become a deal breaker then I would first trade the
> HEAP_LATEST_TUPLE bit and then HEAP_WARM_UPDATED just from correctness
> perspective.

I don't think it makes sense to try and save bits and add complexity
when we have no idea if we will ever use them, but again, I am back to
my original question of whether we have done sufficient research, and if
everyone says "yes", I am find with that.

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

Bruce Momjian wrote:

> I don't think it makes sense to try and save bits and add complexity
> when we have no idea if we will ever use them,

If we find ourselves in dire need of additional bits, there is a known
mechanism to get back 2 bits from old-style VACUUM FULL.  I assume that
the reason nobody has bothered to write the code for that is that
there's no *that* much interest.

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

On Tue, Mar 21, 2017 at 04:43:58PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> 
> > I don't think it makes sense to try and save bits and add complexity
> > when we have no idea if we will ever use them,
> 
> If we find ourselves in dire need of additional bits, there is a known
> mechanism to get back 2 bits from old-style VACUUM FULL.  I assume that
> the reason nobody has bothered to write the code for that is that
> there's no *that* much interest.

We have no way of tracking if users still have pages that used the bits
via pg_upgrade before they were removed.

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

Bruce Momjian wrote:
> On Tue, Mar 21, 2017 at 04:43:58PM -0300, Alvaro Herrera wrote:
> > Bruce Momjian wrote:
> > 
> > > I don't think it makes sense to try and save bits and add complexity
> > > when we have no idea if we will ever use them,
> > 
> > If we find ourselves in dire need of additional bits, there is a known
> > mechanism to get back 2 bits from old-style VACUUM FULL.  I assume that
> > the reason nobody has bothered to write the code for that is that
> > there's no *that* much interest.
> 
> We have no way of tracking if users still have pages that used the bits
> via pg_upgrade before they were removed.

Yes, that's exactly the code that needs to be written.

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

On Tue, Mar 21, 2017 at 04:56:16PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Tue, Mar 21, 2017 at 04:43:58PM -0300, Alvaro Herrera wrote:
> > > Bruce Momjian wrote:
> > > 
> > > > I don't think it makes sense to try and save bits and add complexity
> > > > when we have no idea if we will ever use them,
> > > 
> > > If we find ourselves in dire need of additional bits, there is a known
> > > mechanism to get back 2 bits from old-style VACUUM FULL.  I assume that
> > > the reason nobody has bothered to write the code for that is that
> > > there's no *that* much interest.
> > 
> > We have no way of tracking if users still have pages that used the bits
> > via pg_upgrade before they were removed.
> 
> Yes, that's exactly the code that needs to be written.

Yes, but once it is written it will take years before those bits can be
used on most installations.

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

On Tue, Mar 21, 2017 at 6:47 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
>
>>
>
> Please find attached rebased patches.
>

Few comments on 0005_warm_updates_v18.patch:

1.
@@ -806,20 +835,35 @@ hashbucketcleanup(Relation rel, Bucket
cur_bucket, Buffer bucket_buf,
{
..
- if (callback && callback(htup, callback_state))
+ if(callback) {
- kill_tuple = true;
-
if (tuples_removed)
- *tuples_removed += 1;
+result = callback(htup, is_warm, callback_state);
+ if (result== IBDCR_DELETE)
+ {
+ kill_tuple = true;
+ if (tuples_removed)
+*tuples_removed += 1;
+ }
+ else if (result ==IBDCR_CLEAR_WARM)
+ {
+ clear_tuple= true;
+ } } else if
(split_cleanup)
..
}

I think this will break the existing mechanism of split cleanup.  We
need to check for split cleanup if the tuple is tuple is not deletable
by the callback.  This is not merely an optimization but a must
condition because we will clear the split cleanup flag after this
bucket is scanned completely.

2.
- PageIndexMultiDelete(page, deletable, ndeletable);
+ /*
+
* Clear the WARM pointers.
+ *
+ * We mustdo this before dealing with the dead items because
+ * PageIndexMultiDelete may move items around to compactify the
+ * array and hence offnums recorded earlierwon't make any sense
+ * after PageIndexMultiDelete is called.
+*/
+ if (nclearwarm > 0)
+ _hash_clear_items(page,clearwarm, nclearwarm);
+
+ /*
+ * And delete the deletableitems
+ */
+ if (ndeletable > 0)
+
PageIndexMultiDelete(page, deletable, ndeletable);

I think this assumes that the items where we need to clear warm flag
are not deletable, otherwise what is the need to clear the flag if we
are going to delete the tuple.  The deletable tuple can have a warm
flag if it is deletable due to split cleanup.

3.
+ /*
+ * HASH indexes compute a hash value of the key and store that in the
+ * index. So
we must first obtain the hash of the value obtained from the
+ * heap and then do a comparison
+*/
+ _hash_convert_tuple(indexRel, values, isnull, values2, isnull2);

I think here, you need to handle the case where heap has a NULL value
as the hash index doesn't contain NULL values, otherwise, the code in
below function can return true which is not right.

4.
+bool
+hashrecheck(Relation indexRel, IndexInfo *indexInfo, IndexTuple indexTuple,
+ Relation heapRel, HeapTuple heapTuple)
{
..
+ att = indexRel->rd_att->attrs[i - 1];
+ if (!datumIsEqual(values2[i - 1], indxvalue, att->attbyval,
+ att->attlen))
+ {
+ equal = false;
+ break;
+ }
..
}

Hash values are always uint32 and attlen can be different for
different datatypes, so I think above doesn't seem to be the right way
to do the comparison.

5.
@@ -274,6 +301,8 @@ hashgettuple(IndexScanDesc scan, ScanDirection dir) OffsetNumber offnum;

ItemPointer current; bool res;
+ IndexTuple itup;
+
 /* Hash
indexes are always lossy since we store only the hash code */ scan->xs_recheck = true;
@@ -316,8
+345,6 @@ hashgettuple(IndexScanDesc scan, ScanDirection dir) offnum <=
maxoffnum; offnum = OffsetNumberNext(offnum)) {
-IndexTuple itup;
-

Why above change?


6.
+ *stats = index_bulk_delete(&ivinfo, *stats,
+lazy_indexvac_phase1, (void *) vacrelstats);
+ ereport(elevel,
+(errmsg("scanned index \"%s\" to remove %d row version, found "
+"%0.f warm pointers, %0.f clear pointers, removed "
+"%0.f warm pointers, removed %0.f clear pointers",
+RelationGetRelationName(indrel),
+ vacrelstats->num_dead_tuples,
+ (*stats)->num_warm_pointers,
+(*stats)->num_clear_pointers,
+(*stats)->warm_pointers_removed,
+ (*stats)->clear_pointers_removed)));
+
+ (*stats)->num_warm_pointers = 0;
+ (*stats)->num_clear_pointers = 0;
+ (*stats)->warm_pointers_removed = 0;
+ (*stats)->clear_pointers_removed = 0;
+ (*stats)->pointers_cleared = 0;
+
+ *stats =index_bulk_delete(&ivinfo, *stats,
+ lazy_indexvac_phase2, (void *)vacrelstats);

To convert WARM chains, we need to do two index passes for all the
indexes.  I think it can substantially increase the random I/O. I
think this can help us in doing more WARM updates, but I don't see how
the downside of that (increased random I/O) will be acceptable for all
kind of cases.


+exists. Since index vacuum may visit these pointers in any order, 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 clear the INDEX_WARM_POINTER flag if that's the
+surviving index pointer.

I think there is some mismatch between README and code.  In README, it
is mentioned that dead pointers will be removed in the second phase,
but I think the first phase code lazy_indexvac_phase1() will also
allow to delete the dead pointers (it can return IBDCR_DELETE which
will allow index am to remove dead items.).  Am I missing something
here?


7.
+ * For CLEAR chains, we just kill the WARM pointer, if it exist,s and keep
+ * the CLEAR pointer.

typo (exist,s)

8.
+/*
+ * lazy_indexvac_phase2() -- run first pass of index vacuum

Shouldn't this be -- run the second pass

9.
- indexInfo); /* index AM may need this */
+indexInfo, /* index AM may need this */
+(modified_attrs != NULL)); /* type of uniqueness check to do */

comment for the last parameter seems to be wrong.

10.
+follow the update chain everytime to the end to see check if this is a WARM
+chain.

"see check" - seems one of those words is sufficient to explain the meaning.

11.
+chain. This simplifies the design and addresses certain issues around
+duplicate scans.

"duplicate scans" - shouldn't be duplicate key scans.

12.
+index on the table, irrespective of whether the key pertaining to the
+index changed or not.

typo.
/index changed/index is changed

13.
+For example, if we have a table with two columns and two indexes on each
+of the column. When a tuple is first inserted the table, we have exactly

typo.
/inserted the table/inserted in the table

14.
+ lp [1]  [2]
+ [1111, aaaa]->[111, bbbb]

Here, after the update, the first column should be 1111.




-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Mar 22, 2017 at 3:44 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
>
> This looks quite weird to me. Obviously these numbers are completely
> non-comparable. Even the time for VACUUM FULL goes up with every run.
>
> May be we can blame it on AWS instance completely, but the pattern in your
> tests looks very similar where the number slowly and steadily keeps going
> up. If you do complete retest but run v18/v19 first and then run master, may
> be we'll see a complete opposite picture?
>

For those tests I have done tests in the order --- <Master, patch18,
patch18, Master> both the time numbers were same. One different thing
I did was I was deleting the data directory between tests and creating
the database from scratch. Unfortunately the machine I tested this is
not available. I will test same with v19 once I get the machine and
report you back.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

On 21 March 2017 at 20:04, Bruce Momjian <bruce@momjian.us> wrote:
> Yes, but once it is written it will take years before those bits can be
> used on most installations.

Well the problem isn't most installations. On most installations it
should be pretty straightforward to check the oldest database xid and
compare that to when the database was migrated to post-9.0. (Actually
there may be some additional code to write but it's just ensuring that
the bits are actually cleared and not just ignored but even so
databases do generally need to be vacuumed more often than on the
order of years though.)

The problem is that somebody tomorrow could upgrade an 8.4 database to
10.0. In general it seems even versions we don't support get extra
support for migrating away from. I assume it's better to help support
upgrading than to continue to have users using unsupported versions...
And even if you're not concerned about 8.4 someone could still upgrade
9.4 for years to come.

It probably does make sense pick a version, say, 10.0, and have it go
out of its way to ensure it cleans up the MOVED_IN/MOVED_OFF so that
we can be sure that any database was pg_upgraded from 10.0+ doesn't
have any left. Then at least we'll know when the bits are available
again.


-- 
greg

On Wed, Mar 22, 2017 at 8:43 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
> Sorry, I did not mean to suggest that you set it up wrongly, I was just
> trying to point out that the test case itself may not be very practical.
That is cool np!, I was just trying to explain why those tests were
made if others wondered about it.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

On Thu, Mar 23, 2017 at 12:19 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
>
>
> On Wed, Mar 22, 2017 at 4:53 PM, Mithun Cy <mithun.cy@enterprisedb.com>
> wrote:
> Ok, no problem. I did some tests on AWS i2.xlarge instance (4 vCPU, 30GB
> RAM, attached SSD) and results are shown below. But I think it is important
> to get independent validation from your side too, just to ensure I am not
> making any mistake in measurement. I've attached naively put together
> scripts which I used to run the benchmark. If you find them useful, please
> adjust the paths and run on your machine.

Looking at your postgresql.conf  JFYI, I have synchronous_commit = off
but same is on with your run (for logged tables) and rest remains
same. Once I get the machine probably next morning, I will run same
tests on v19.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

On Wed, Mar 22, 2017 at 4:06 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Mar 21, 2017 at 6:47 PM, Pavan Deolasee
> <pavan.deolasee@gmail.com> wrote:
>>
>>>
>>
>> Please find attached rebased patches.
>>
>
> Few comments on 0005_warm_updates_v18.patch:
>

Few more comments on 0005_warm_updates_v18.patch:
1.
@@ -234,6 +241,25 @@ 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. Also build and cache IndexInfo which is used by
+ * amrecheck routine.
+ *
+ * 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;
+ scan->indexInfo = BuildIndexInfo(indexRelation);
+ }
+

Don't we need to do this rechecking during parallel scans?  Also what
about bitmap heap scans?

2.
+++ b/src/backend/access/nbtree/nbtinsert.c
-typedef struct

Above change is not require.

3.
+_bt_clear_items(Page page, OffsetNumber *clearitemnos, uint16 nclearitems)
+void _hash_clear_items(Page page, OffsetNumber *clearitemnos,
+   uint16 nclearitems)

Both the above functions look exactly same, isn't it better to have a
single function like page_clear_items?  If you want separation for
different index types, then we can have one common function which can
be called from different index types.

4.
- if (callback(htup, callback_state))
+ flags = ItemPointerGetFlags(&itup->t_tid);
+ is_warm = ((flags & BTREE_INDEX_WARM_POINTER) != 0);
+
+ if (is_warm)
+ stats->num_warm_pointers++;
+ else
+ stats->num_clear_pointers++;
+
+ result = callback(htup, is_warm, callback_state);
+ if (result == IBDCR_DELETE)
+ {
+ if (is_warm)
+ stats->warm_pointers_removed++;
+ else
+ stats->clear_pointers_removed++;

The patch looks to be inconsistent in collecting stats for btree and
hash.  I don't see above stats are getting updated in hash index code.

5.
+btrecheck(Relation indexRel, IndexInfo *indexInfo, IndexTuple indexTuple,
+ Relation heapRel, HeapTuple heapTuple)
{
..
+ if (!datumIsEqual(values[i - 1], indxvalue, att->attbyval,
+ att->attlen))
..
}

Will this work if the index is using non-default collation?

6.
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -390,83 +390,9 @@ btree_xlog_vacuum(XLogReaderState *record)
-#ifdef UNUSED xl_btree_vacuum *xlrec = (xl_btree_vacuum *) XLogRecGetData(record);
 /*
- * This section of code is thought to be no longer needed, after analysis
- * of the calling paths. It is retained to allow the code to be reinstated
- * if a flaw is revealed in that thinking.
- *
..

Why does this patch need to remove the above code under #ifdef UNUSED

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Mar 23, 2017 at 12:19 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
>
> Ok, no problem. I did some tests on AWS i2.xlarge instance (4 vCPU, 30GB
> RAM, attached SSD) and results are shown below. But I think it is important
> to get independent validation from your side too, just to ensure I am not
> making any mistake in measurement. I've attached naively put together
> scripts which I used to run the benchmark. If you find them useful, please
> adjust the paths and run on your machine.
>
> I reverted back to UNLOGGED table because with WAL the results looked very
> weird (as posted earlier) even when I was taking a CHECKPOINT before each
> set and had set max_wal_size and checkpoint_timeout high enough to avoid any
> checkpoint during the run. Anyways, that's a matter of separate
> investigation and not related to this patch.
>
> I did two kinds of tests.
> a) update last column of the index
> b) update second column of the index
>
> v19 does considerably better than even master for the last column update
> case and pretty much inline for the second column update test. The reason is
> very clear because v19 determines early in the cycle that the buffer is
> already full and there is very little chance of doing a HOT update on the
> page. In that case, it does not check any columns for modification.
>

That sounds like you are dodging the actual problem.  I mean you can
put that same PageIsFull() check in master code as well and then you
will most probably again see the same regression.  Also, I think if we
test at fillfactor 80 or 75 (which is not unrealistic considering an
update-intensive workload), then we might again see regression.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Mar 23, 2017 at 3:44 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
> On Wed, Mar 22, 2017 at 4:06 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>
>
>> 3.
>> + /*
>> + * HASH indexes compute a hash value of the key and store that in the
>> + * index. So
>> we must first obtain the hash of the value obtained from the
>> + * heap and then do a comparison
>> +
>>  */
>> + _hash_convert_tuple(indexRel, values, isnull, values2, isnull2);
>>
>> I think here, you need to handle the case where heap has a NULL value
>> as the hash index doesn't contain NULL values, otherwise, the code in
>> below function can return true which is not right.
>>
>
> I think we can simply conclude hashrecheck has failed the equality if the
> heap has NULL value because such a tuple should not have been reached via
> hash index unless a non-NULL hash key was later updated to a NULL key,
> right?
>

Right.

>>
>>
>> 6.
>> + *stats = index_bulk_delete(&ivinfo, *stats,
>> +lazy_indexvac_phase1, (void *) vacrelstats);
>> + ereport(elevel,
>> +(errmsg("scanned index \"%s\" to remove %d row version, found "
>> +"%0.f warm pointers, %0.f clear pointers, removed "
>> +"%0.f warm pointers, removed %0.f clear pointers",
>> +RelationGetRelationName(indrel),
>> + vacrelstats->num_dead_tuples,
>> + (*stats)->num_warm_pointers,
>> +(*stats)->num_clear_pointers,
>> +(*stats)->warm_pointers_removed,
>> + (*stats)->clear_pointers_removed)));
>> +
>> + (*stats)->num_warm_pointers = 0;
>> + (*stats)->num_clear_pointers = 0;
>> + (*stats)->warm_pointers_removed = 0;
>> + (*stats)->clear_pointers_removed = 0;
>> + (*stats)->pointers_cleared = 0;
>> +
>> + *stats =index_bulk_delete(&ivinfo, *stats,
>> + lazy_indexvac_phase2, (void *)vacrelstats);
>>
>> To convert WARM chains, we need to do two index passes for all the
>> indexes.  I think it can substantially increase the random I/O. I
>> think this can help us in doing more WARM updates, but I don't see how
>> the downside of that (increased random I/O) will be acceptable for all
>> kind of cases.
>>
>
> Yes, this is a very fair point. The way I proposed to address this upthread
> is by introducing a set of threshold/scale GUCs specific to WARM. So users
> can control when to invoke WARM cleanup. Only if the WARM cleanup is
> required, we do 2 index scans. Otherwise vacuum will work the way it works
> today without any additional overhead.
>

I am not sure on what basis user can set such parameters, it will be
quite difficult to tune those parameters.  I think the point is
whatever threshold we keep, once that is crossed, it will perform two
scans for all the indexes.  IIUC, this conversion of WARM chains is
required so that future updates can be WARM or is there any other
reason?  I see this as a big penalty for future updates.

> We already have some intelligence to skip the second index scan if we did
> not find any WARM candidate chains during the first heap scan. This should
> take care of majority of the users who never update their indexed columns.
> For others, we need either a knob or some built-in way to deduce whether to
> do WARM cleanup or not.
>
> Does that seem worthwhile?
>

Is there any consensus on your proposal, because I feel this needs
somewhat broader discussion, me and you can't take a call on this
point.  I request others also to share their opinion on this point.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi Pavan,
On Thu, Mar 23, 2017 at 12:19 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
> Ok, no problem. I did some tests on AWS i2.xlarge instance (4 vCPU, 30GB
> RAM, attached SSD) and results are shown below. But I think it is important
> to get independent validation from your side too, just to ensure I am not
> making any mistake in measurement. I've attached naively put together
> scripts which I used to run the benchmark. If you find them useful, please
> adjust the paths and run on your machine.

I did a similar test appears. Your v19 looks fine to me, it does not
cause any regression, On the other hand, I also ran tests reducing
table fillfactor to 80 there I can see a small regression 2-3% in
average when updating col2 and on updating col9 again I do not see any
regression.

-- 
Thanks and Regards
Mithun C Y
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 Fri, Mar 24, 2017 at 12:25 AM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
>
>
> On Thu, Mar 23, 2017 at 7:53 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>
>> >
>>
>> I am not sure on what basis user can set such parameters, it will be
>> quite difficult to tune those parameters.  I think the point is
>> whatever threshold we keep, once that is crossed, it will perform two
>> scans for all the indexes.
>
>
> Well, that applies to even vacuum parameters, no?
>

I don't know how much we can directly compare the usability of the new
parameters you are proposing here to existing parameters.

> The general sense I've got
> here is that we're ok to push some work in background if it helps the
> real-time queries, and I kinda agree with that.
>

I don't think we can define this work as "some" work, it can be a lot
of work depending on the number of indexes.  Also, I think for some
cases it will generate maintenance work without generating benefit.
For example, when there is one index on a table and there are updates
for that index column.

> Having said that, I see many ways we can improve on this later. Like we can
> track somewhere else information about tuples which may have received WARM
> updates (I think it will need to be a per-index bitmap or so) and use that
> to do WARM chain conversion in a single index pass.
>

Sure, if we have some way to do it in a single pass or does most of
the time in foreground process (like we have some dead marking idea
for indexes), then it would have been better.

> But this is clearly not
> PG 10 material.
>

I don't see much discussion about this aspect of the patch, so not
sure if it is acceptable to increase the cost of vacuum.  Now, I don't
know if your idea of GUC's make it such that the additional cost will
occur seldom and this additional pass has a minimal impact which will
make it acceptable.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com