HOT patch - version 14

Pavan Deolasee wrote:

Please see the version 14 of HOT patch attached. I have also
attached a differential patch from the last version posted. This
now includes support for partial and expression indexes. As per
the discussion, we collect all the index columns appearing in
the partial index predicates and/or expression index expressions.
If any of these columns are being updated, the update does not
qualify for HOT update.

I see that you have a separate bitmapset to keep track of indexes on
system attributes. But having an index on a system attribute doesn't
make any sense, does it?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki@enterprisedb.com> writes:

I see that you have a separate bitmapset to keep track of indexes on
system attributes. But having an index on a system attribute doesn't
make any sense, does it?

Counterexample: OID.

regards, tom lane

On 8/20/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Heikki Linnakangas <heikki@enterprisedb.com> writes:

I see that you have a separate bitmapset to keep track of indexes on
system attributes. But having an index on a system attribute doesn't
make any sense, does it?

Counterexample: OID.

Right. Further, we allow creating indexes on system attributes. So we
must support those.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

Please see the version 14 of HOT patch attached.

I expected to find either a large new README, or some pretty substantial
additions to existing README files, to document how this all works.
The comments included do not represent nearly enough documentation.

One thing I was unable to discern from the comments is how CREATE INDEX
can work at all. A new index might mean that tuples that could formerly
legally be part of the same hot-chain no longer can. I can't find any
code here that looks like it's addressing that. I also don't think I
believe the reasoning for not indexing DELETE_IN_PROGRESS hot-updated
tuples: what if the index completion commits, but the concurrent delete
rolls back? Then you've got a valid tuple that's not in the index.

I also took this opportunity to remove the modularity invasion caused
by heap_check_idxupdate() since it was using resultRelInfo. We now
build the list of attributes that must be checked to satisfy HOT update.
This list includes all the index columns, columns in the partial index
predicates and expression index expressions and is built in the
executor.

The executor is the wrong place for that: I'm not sure why you think
that making heapam depend on the executor is a modularity improvement.
Furthermore this approach requires recalculating the list during
each query, which is wasteful when it could only change during schema
updates. I'd suggest making the relcache responsible for computing and
saving this data, along the same lines as RelationGetIndexList().
(That also means that heapam can get the data from the relcache, saving
a lot of API-bloating from passing around Attrids explicitly.)
Also, rather than inventing Attrids, I'd suggest just using one
Bitmapset with the convention that its contents are offset by
FirstLowInvalidHeapAttributeNumber.

The redefinition of the value of MaxHeapTuplesPerPage seems pretty
ugly and unprincipled. I think it'd be better to leave it as-is,
and just enforce that we don't allow more than that many line pointers
on a heap page (as indeed you have to do anyway, so it's not clear
what the change is buying).

Is it really necessary to add hot_update to xl_heap_update? Seems the
information should be available from the tuple header fields.

Have you demonstrated that the "prune_hard" logic is worth its weight?
Considering that many of us want to drop VACUUM FULL, adding more
complexity to it doesn't seem like a profitable use of time.

Is it really safe, or productive, to run heap_page_prune when the buffer
is not locked for cleanup (ie, there are other people with pins on it)?
Even if it's safe, ISTM what you're mostly accomplishing there is to
expend a lot of cycles while holding exclusive lock on the page, when
there is good reason to think that you're blocking other people who are
interested in using the page. Eliminating the separation between that
and cleanup would also allow eliminating the separate "PD_FRAGMENTED"
page state.

PlanSetValidForThisTransaction is completely bogus --- it's not
re-entrant, and it needs to be. I think you need some state in
PlannerGlobal instead.

rd_avgfsm seems fairly bogus ... when does it get updated?

regards, tom lane

On 8/30/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

Please see the version 14 of HOT patch attached.

I expected to find either a large new README, or some pretty substantial
additions to existing README files, to document how this all works.
The comments included do not represent nearly enough documentation.

I shall take that up. There are couple of documents posted by Heikki and
Greg,
apart from several email threads and original design doc by Simon. I shall
consolidate everything in a single README.

One thing I was unable to discern from the comments is how CREATE INDEX

can work at all. A new index might mean that tuples that could formerly
legally be part of the same hot-chain no longer can. I can't find any
code here that looks like it's addressing that.

You are right - a new index might mean that an existing HOT chain
is broken as far as the new index is concerned. The way we address
that is by indexing the root tuple of the chain, but the index key is
extracted from the last tuple in the chain. The index is marked "unusable"
for all those existing transactions which can potentially see any
intermediate
tuples in the chain.

Please see this document written by Greg Stark. He has nicely summarized
how CREATE INDEX and CREATE INDEX CONCURRENTLY works with HOT.

http://archives.postgresql.org/pgsql-patches/2007-07/msg00360.php

I also don't think I

believe the reasoning for not indexing DELETE_IN_PROGRESS hot-updated
tuples: what if the index completion commits, but the concurrent delete
rolls back? Then you've got a valid tuple that's not in the index.

Since CREATE INDEX works with ShareLock on the relation, we can
safely assume that we can't find any DELETE_IN_PROGRESS tuples except
those deleted by our own transaction. The only exception is system relation,
but we don't do HOT updates on system relation. Given that, it seems OK
to me to ignore these tuples because if the transaction aborts,
CREATE INDEX is aborted as well. Am I overlooking something here ?
There is a comment to this regard in the current code as well.

I also took this opportunity to remove the modularity invasion caused

by heap_check_idxupdate() since it was using resultRelInfo. We now
build the list of attributes that must be checked to satisfy HOT update.
This list includes all the index columns, columns in the partial index
predicates and expression index expressions and is built in the
executor.

The executor is the wrong place for that: I'm not sure why you think
that making heapam depend on the executor is a modularity improvement.

Earlier (in the previous version of HOT patch) we were passing ResultRelInfo
to heap_update, which was more ugly. The comment is in that context :-)

Furthermore this approach requires recalculating the list during

each query, which is wasteful when it could only change during schema
updates. I'd suggest making the relcache responsible for computing and
saving this data, along the same lines as RelationGetIndexList().
(That also means that heapam can get the data from the relcache, saving
a lot of API-bloating from passing around Attrids explicitly.)
Also, rather than inventing Attrids, I'd suggest just using one
Bitmapset with the convention that its contents are offset by
FirstLowInvalidHeapAttributeNumber.

I liked all these suggestions. I know I thought about computing
the attribute list in relcache, but probably avoided to keep things simple.
I shall make these changes.

The redefinition of the value of MaxHeapTuplesPerPage seems pretty

ugly and unprincipled. I think it'd be better to leave it as-is,
and just enforce that we don't allow more than that many line pointers
on a heap page (as indeed you have to do anyway, so it's not clear
what the change is buying).

The only reason to redefine MaxHeapTuplesPerPage to higher side is
because HOT allows us to accommodate more tuples per page because
of redirect-dead line pointers. For a table with sufficiently large tuples,
the original bound would work well, but for very small tuples - we might
hit the line pointer limit even if there is free space available in the
page. Doubling the value is chosen as a balance between heap page
utilization, line pointer bloating and overhead for bitmap scans. But
I agree that the factor choice is rather arbitrary.

Is it really necessary to add hot_update to xl_heap_update? Seems the
information should be available from the tuple header fields.

I think its not necessary. The reason I did that way because the new block
might be backed up in the WAL record and hence we might have recorded
the new tuple infomasks. But we can surely handle that corner case. I shall
fix this.

Have you demonstrated that the "prune_hard" logic is worth its weight?

Considering that many of us want to drop VACUUM FULL, adding more
complexity to it doesn't seem like a profitable use of time.

The prune_hard code is lot more simple in this version (it was horrible
before Heikki and I reworked the pruning logic). We just follow the HOT
chain to the last definitely DEAD tuple and remove all tuples preceding
that. This simplifies the second phase of VACUUM FULL since
we don't need to handle any broken HOT chains because of intermediate
DEAD tuples. Also, prune_hard allows us to cleanup the redirected
line pointers.

Another reason for doing this is to avoid any significant changes to
VACUUM FULL code since the code itself is complex and we might
just drop this in future. But until that time, we need to make sure that
HOT works fine with VACUUM FULL.

Is it really safe, or productive, to run heap_page_prune when the buffer

is not locked for cleanup (ie, there are other people with pins on it)?

It looks safe to me. We don't move tuples around during chain pruning.
We only fix the HOT chains by removing intermediate DEAD tuples.
Other places where we follow HOT chains, we do so while holding
at-least SHARE lock on the buffer. So there are no race conditions here.

A place where this bite me is that the heap-scan with SnapshotAny
may return a tuple which gets pruned (and marked ~LP_USED)
before the caller can see it. Fortunately there are only limited users
of SnapshotAny. We make sure that IndexBuildHeapScan confirms that
the ItemIdIsValid() after acquiring lock on the buffer. Cluster works with
exclusive lock on the relation and hence there is no concurrent pruning
possible.

Even if it's safe, ISTM what you're mostly accomplishing there is to

expend a lot of cycles while holding exclusive lock on the page, when
there is good reason to think that you're blocking other people who are
interested in using the page. Eliminating the separation between that
and cleanup would also allow eliminating the separate "PD_FRAGMENTED"
page state.

The reason we did it that way because repairing fragmentation seems
much more costly that pruning. Please note that we prune a single
chain during index fetch. Its only for heap-scans (and VACUUM) that
we try to prune all chains in the page. So unless we are doing heap-scan,
I am not sure if we are spending too much time holding the exclusive
lock. I agree we don't have any specific numbers to prove that though.

Another reasoning behind separating these two steps is: pruning
requires exclusive lock whereas repairing fragmentation requires
cleanup lock. Also we want to prune the chains aggressively
because the visible tuple is most likely at the end of the chain. Longer
the chain, greater the cost to fetch the tuple.

PlanSetValidForThisTransaction is completely bogus --- it's not

re-entrant, and it needs to be. I think you need some state in
PlannerGlobal instead.

I was not happy with this - frankly I don't understand the planning code
much. Thanks for pointing me to the appropriate code. I shall try fix this,
unless you would like to do it yourself.

rd_avgfsm seems fairly bogus ... when does it get updated?

Yeah, its not the best solution. What I wanted to do is delay repairing
page fragmentation as far as possible. But I noticed that fsmrel->avgRequest
(rd_avgfsm is initialized to it) starts with a small value (I guess 256).
For a table with large tuples, we may not repair fragmentation at all, thus
causing unnecessary COLD updates.

rd_avgfsm is updated when the relcache is rebuilt. Since we don't
send out any relcache invalidation when fsmrel->avgRequest changes,
rd_avgfsm may never get updated in a session.

Should we just revert to checking if the free space is less than a certain
percentage of BLCKSZ and trigger defragmentation ? Or may be do a
combination of both ?

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

Please see the version 14 of HOT patch attached.

I expected to find either a large new README, or some pretty substantial
additions to existing README files, to document how this all works.
The comments included do not represent nearly enough documentation.

The Heikki and I posted a two-part README of sorts:

http://archives.postgresql.org/pgsql-patches/2007-07/msg00142.php
http://archives.postgresql.org/pgsql-patches/2007-07/msg00360.php

One thing I was unable to discern from the comments is how CREATE INDEX
can work at all. A new index might mean that tuples that could formerly
legally be part of the same hot-chain no longer can. I can't find any
code here that looks like it's addressing that.

This is one of the weirdest things in HOT and one of the hardest problems it
faced. The best solution proposed was to index the head of the HOT chain and
just ban older transactions which might be able to see the older versions fro
using the index.

That's the purpose of the indcreatexid column. It's set in
index_set_createxid() and checked in get_relation_info().

I also don't think I believe the reasoning for not indexing
DELETE_IN_PROGRESS hot-updated tuples: what if the index completion commits,
but the concurrent delete rolls back? Then you've got a valid tuple that's
not in the index.

You're talking about the concurrent index build case? Wouldn't the second pass
pick up that tuple? I have to look back at it to see for sure.

The redefinition of the value of MaxHeapTuplesPerPage seems pretty
ugly and unprincipled. I think it'd be better to leave it as-is,
and just enforce that we don't allow more than that many line pointers
on a heap page (as indeed you have to do anyway, so it's not clear
what the change is buying).

Note that in a heavily updated table basically every tuple will have two line
pointers, the head which just redirects to another line pointer, and the line
pointer for the actual tuple. It's hard to get rid of the actual head line
pointer without declaring that the tid might sometimes not change after an
update.

Not sure what this translates to for MaxHeapTuplesPerPage though.

The rest I know less about and will leave to Pavan and Heikki (or anyone else
who was following those details more closely).

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

"Gregory Stark" <stark@enterprisedb.com> writes:

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

Please see the version 14 of HOT patch attached.

I expected to find either a large new README, or some pretty substantial
additions to existing README files, to document how this all works.
The comments included do not represent nearly enough documentation.

The Heikki and I posted a two-part README of sorts:

Er, editing error there. Has a ring to it though.

http://archives.postgresql.org/pgsql-patches/2007-07/msg00142.php
http://archives.postgresql.org/pgsql-patches/2007-07/msg00360.php

...

I also don't think I believe the reasoning for not indexing
DELETE_IN_PROGRESS hot-updated tuples: what if the index completion commits,
but the concurrent delete rolls back? Then you've got a valid tuple that's
not in the index.

You're talking about the concurrent index build case? Wouldn't the second pass
pick up that tuple? I have to look back at it to see for sure.

Sorry, that's misguided. The concurrent index build uses snapshots now so it
can't see DELETE_IN_PROGRESS. And the non-concurrent index build has an lock
so it ought to be back to the way it was before I messed with it where there
was an assert against finding *_IN_PROGRESS (except as Pavan points out in the
same transaction).

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

You are right - a new index might mean that an existing HOT chain
is broken as far as the new index is concerned. The way we address
that is by indexing the root tuple of the chain, but the index key is
extracted from the last tuple in the chain. The index is marked "unusable"
for all those existing transactions which can potentially see any
intermediate tuples in the chain.

I don't think that works --- what if the last tuple in the chain isn't
committed good yet? If its inserter ultimately rolls back, you've
indexed the wrong value.

Please see this document written by Greg Stark. He has nicely summarized
how CREATE INDEX and CREATE INDEX CONCURRENTLY works with HOT.
http://archives.postgresql.org/pgsql-patches/2007-07/msg00360.php

Isn't the extra machination for C.I.C. just useless complication?
What's the point of avoiding creation of new broken HOT chains when
you still have to deal with existing ones?

I also don't think I
believe the reasoning for not indexing DELETE_IN_PROGRESS hot-updated
tuples: what if the index completion commits, but the concurrent delete
rolls back? Then you've got a valid tuple that's not in the index.

Since CREATE INDEX works with ShareLock on the relation, we can
safely assume that we can't find any DELETE_IN_PROGRESS tuples except
those deleted by our own transaction. The only exception is system relation,
but we don't do HOT updates on system relation.

That chain of reasoning is way too shaky for me. Essentially what
you're saying is you'll promise not to corrupt non-system indexes.
Nor am I really thrilled about having to disable HOT for system
catalogs.

The only reason to redefine MaxHeapTuplesPerPage to higher side is
because HOT allows us to accommodate more tuples per page because
of redirect-dead line pointers.

No, it doesn't allow more tuples per page. It might mean there can be
more line pointers than that on a page, but not more actual tuples.
The question is whether there is any real use in allowing more line
pointers than that --- the limit is already unrealistically high,
since it assumes no data content in any of the tuples. If there is a
rationale for it then you should invent a different constant
MaxLinePointersPerPage or some such, but I really think there's no
point.

Doubling the value is chosen as a balance between heap page
utilization, line pointer bloating and overhead for bitmap scans.

I'd say it allows a ridiculous amount of line pointer bloat.

Even if it's safe, ISTM what you're mostly accomplishing there is to
expend a lot of cycles while holding exclusive lock on the page, when
there is good reason to think that you're blocking other people who are
interested in using the page. Eliminating the separation between that
and cleanup would also allow eliminating the separate "PD_FRAGMENTED"
page state.

The reason we did it that way because repairing fragmentation seems
much more costly that pruning. Please note that we prune a single
chain during index fetch. Its only for heap-scans (and VACUUM) that
we try to prune all chains in the page. So unless we are doing heap-scan,
I am not sure if we are spending too much time holding the exclusive
lock. I agree we don't have any specific numbers to prove that though.

If you don't have numbers proving that this extra complication has a
benefit, I'd vote for simplifying it. The SnapshotAny case is going to
bite other people besides you in future.

Another reasoning behind separating these two steps is: pruning
requires exclusive lock whereas repairing fragmentation requires
cleanup lock.

This is nonsense. Those are the same lock. If you have the former and
not the latter, it just means that you *know* there is contention for
the page. It seems to me that performing optional maintenance work in
such a situation is completely wrong.

regards, tom lane

On 8/30/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

You are right - a new index might mean that an existing HOT chain
is broken as far as the new index is concerned. The way we address
that is by indexing the root tuple of the chain, but the index key is
extracted from the last tuple in the chain. The index is marked

"unusable"

for all those existing transactions which can potentially see any
intermediate tuples in the chain.

I don't think that works --- what if the last tuple in the chain isn't
committed good yet? If its inserter ultimately rolls back, you've
indexed the wrong value.

I am confused. How could we get ShareLock on the relation while
there is some open transaction which has inserted a tuple in the
table ? (Of course, I am not considering the system tables here)
If the transaction which inserted the last tuple in the chain is already
aborted, we are not indexing that tuple (even if that tuple is at the
end). We would rather index the last committed-good tuple in the
chain. Running the tuples with HeapTupleSatisfiesVacuum guarantees
that. Isn't it ?

Please see this document written by Greg Stark. He has nicely summarized

how CREATE INDEX and CREATE INDEX CONCURRENTLY works with HOT.
http://archives.postgresql.org/pgsql-patches/2007-07/msg00360.php

Isn't the extra machination for C.I.C. just useless complication?
What's the point of avoiding creation of new broken HOT chains when
you still have to deal with existing ones?

IMHO the extra step in C.I.C simplifies the index build. The
transaction-waits
takes care of the existing broken chains and the early catalog entry for
the index helps us avoid creating new broken chains. I am not against
doing it a different way, but this is the best solution we could come up
when we worked on CIC.

I also don't think I

believe the reasoning for not indexing DELETE_IN_PROGRESS hot-updated
tuples: what if the index completion commits, but the concurrent delete
rolls back? Then you've got a valid tuple that's not in the index.

Since CREATE INDEX works with ShareLock on the relation, we can
safely assume that we can't find any DELETE_IN_PROGRESS tuples except
those deleted by our own transaction. The only exception is system

relation,

but we don't do HOT updates on system relation.

That chain of reasoning is way too shaky for me. Essentially what
you're saying is you'll promise not to corrupt non-system indexes.
Nor am I really thrilled about having to disable HOT for system
catalogs.

I am not against supporting HOT for system catalogs. But IMHO
its not a strict requirements because system catalogs are small, less
frequently updated and HOT adds little value to them. If we don't have
a generic solution which works for system and non-system tables,
thats the best we can get. We can start with non-system
tables and expand its scope later.

The only reason to redefine MaxHeapTuplesPerPage to higher side is

because HOT allows us to accommodate more tuples per page because
of redirect-dead line pointers.

No, it doesn't allow more tuples per page. It might mean there can be
more line pointers than that on a page, but not more actual tuples.
The question is whether there is any real use in allowing more line
pointers than that --- the limit is already unrealistically high,
since it assumes no data content in any of the tuples. If there is a
rationale for it then you should invent a different constant
MaxLinePointersPerPage or some such, but I really think there's no
point.

I agree. I think the current limit on MaxHeapTuplesPerPage is sufficiently
large. May be we can keep it the same and tune it later if we have numbers
to prove its worthiness.

Doubling the value is chosen as a balance between heap page

utilization, line pointer bloating and overhead for bitmap scans.

I'd say it allows a ridiculous amount of line pointer bloat.

OK. Lets keep MaxHeapTuplesPerPage unchanged.

Even if it's safe, ISTM what you're mostly accomplishing there is to

expend a lot of cycles while holding exclusive lock on the page, when
there is good reason to think that you're blocking other people who are
interested in using the page. Eliminating the separation between that
and cleanup would also allow eliminating the separate "PD_FRAGMENTED"
page state.

The reason we did it that way because repairing fragmentation seems
much more costly that pruning. Please note that we prune a single
chain during index fetch. Its only for heap-scans (and VACUUM) that
we try to prune all chains in the page. So unless we are doing

heap-scan,

I am not sure if we are spending too much time holding the exclusive
lock. I agree we don't have any specific numbers to prove that though.

If you don't have numbers proving that this extra complication has a
benefit, I'd vote for simplifying it. The SnapshotAny case is going to
bite other people besides you in future.

OK. So if I get you correctly, you are suggesting to acquire cleanup lock.
If we don't get that, we don't to any maintenance work. Otherwise, we prune
and repair fragmentation in one go.

A related question is: should we always prune all chains in the
page ? I guess if we are going to repair fragmentation, it would make
more sense to do that.

Another reasoning behind separating these two steps is: pruning

requires exclusive lock whereas repairing fragmentation requires
cleanup lock.

This is nonsense. Those are the same lock. If you have the former and
not the latter, it just means that you *know* there is contention for
the page. It seems to me that performing optional maintenance work in
such a situation is completely wrong.

Oh yes, they are the same lock. The difference is the chances of getting
one is more than the other. But I agree with your argument about the
contention
and maintenance work. I think we can do what you are suggesting and
then fine tune it.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

On 8/30/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't think that works --- what if the last tuple in the chain isn't
committed good yet? If its inserter ultimately rolls back, you've
indexed the wrong value.

I am confused. How could we get ShareLock on the relation while
there is some open transaction which has inserted a tuple in the
table ? (Of course, I am not considering the system tables here)

Not if someone else releases lock before committing. Which I remind you
is a programming technique we use quite a lot with respect to the system
catalogs. I'm not prepared to guarantee that there is no code path in
core (much less in contrib or third-party addons) that doesn't do it on
a user table; even less that no such path will ever appear in future.
As things stand it's a pretty harmless error --- the worst consequence
I know of is that a VACUUM FULL starting right then might bleat and
refuse to do shrinking. What you propose is to turn the case into a
cause of silent index corruption.

A more robust solution would be to wait out the source transaction if
CREATE INDEX comes across an INSERT_IN_PROGRESS or DELETE_IN_PROGRESS
HOT tuple. Or you could throw an error, since it's not really likely to
happen (except maybe in a system catalog REINDEX operation). But the
way the patch approaches this at the moment is far too fragile IMHO.

If we approach it this way, we might also be able to jettison some of
the other complexity such as idxcreatexid.

Isn't the extra machination for C.I.C. just useless complication?
What's the point of avoiding creation of new broken HOT chains when
you still have to deal with existing ones?

IMHO the extra step in C.I.C simplifies the index build.

If you make the change suggested above, I think you don't need to do
things differently in C.I.C.

I also don't think I
believe the reasoning for not indexing DELETE_IN_PROGRESS hot-updated
tuples: what if the index completion commits, but the concurrent delete
rolls back? Then you've got a valid tuple that's not in the index.

Since CREATE INDEX works with ShareLock on the relation, we can
safely assume that we can't find any DELETE_IN_PROGRESS tuples except
those deleted by our own transaction. The only exception is system
relation, but we don't do HOT updates on system relation.

Same issue as above: this makes correctness utterly dependent on nobody
releasing locks early.

OK. So if I get you correctly, you are suggesting to acquire cleanup lock.
If we don't get that, we don't to any maintenance work. Otherwise, we prune
and repair fragmentation in one go.

Yeah, that's what I'm thinking; then there's no need to track a separate
page state where we've pruned but not defragmented.

regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

Isn't the extra machination for C.I.C. just useless complication?
What's the point of avoiding creation of new broken HOT chains when
you still have to deal with existing ones?

IMHO the extra step in C.I.C simplifies the index build.

If you make the change suggested above, I think you don't need to do
things differently in C.I.C.

It seems to me if you wait out transactions as you come across them you could
end up waiting a whole lot longer than the way it works now where it waits
them all out at the end of the first pass.

OK. So if I get you correctly, you are suggesting to acquire cleanup lock.
If we don't get that, we don't to any maintenance work. Otherwise, we prune
and repair fragmentation in one go.

Yeah, that's what I'm thinking; then there's no need to track a separate
page state where we've pruned but not defragmented.

Note that you still need to do it in two steps, you just get to postpone the
pruning work to the same point in time as the defragmenting. You can never
"acquire" the cleanup lock when it comes time to insert a new update version
since the executor still holds references to the original tuple.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Gregory Stark <stark@enterprisedb.com> writes:

If you make the change suggested above, I think you don't need to do
things differently in C.I.C.

It seems to me if you wait out transactions as you come across them you could
end up waiting a whole lot longer than the way it works now where it waits
them all out at the end of the first pass.

I think you might have misread that --- I intended to say that C.I.C
could still work the way it does today, not that it would be exactly
like regular CREATE INDEX. The wait-out business should only be needed
for a regular CREATE INDEX, and in that case there's no cumulative
waiting effect because no new conflicting transactions are coming in.
C.I.C. should be able to fix things up in its second pass instead of
waiting during the first one.

regards, tom lane

Tom Lane escribi�:

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

On 8/30/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't think that works --- what if the last tuple in the chain isn't
committed good yet? If its inserter ultimately rolls back, you've
indexed the wrong value.

I am confused. How could we get ShareLock on the relation while
there is some open transaction which has inserted a tuple in the
table ? (Of course, I am not considering the system tables here)

Not if someone else releases lock before committing.

FWIW, a red flag raised for me here, though maybe it is irrelevant or
unimportant. Currently, VACUUM acquires an exclusive lock for
truncating the table. The lock is kept till commit. However I am
proposing that it be released before commit.

Now, VACUUM never inserts rows. But I don't claim I understand what's
going on here.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> writes:

Tom Lane escribi�:

Not if someone else releases lock before committing.

FWIW, a red flag raised for me here, though maybe it is irrelevant or
unimportant. Currently, VACUUM acquires an exclusive lock for
truncating the table. The lock is kept till commit. However I am
proposing that it be released before commit.

I think that's all right, because it's dealing with a different set of
concerns. AFAICS the only issue for truncation is to prevent physical
access to the blocks in question until we can get rid of them. Once
they're gone, if there wasn't an active seqscan (with an
already-established notion of the max block number to scan to), there
would be no reason for anyone to try to touch them.

regards, tom lane

On 8/31/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:

Not if someone else releases lock before committing. Which I remind you
is a programming technique we use quite a lot with respect to the system
catalogs. I'm not prepared to guarantee that there is no code path in
core (much less in contrib or third-party addons) that doesn't do it on
a user table; even less that no such path will ever appear in future.
As things stand it's a pretty harmless error --- the worst consequence
I know of is that a VACUUM FULL starting right then might bleat and
refuse to do shrinking. What you propose is to turn the case into a
cause of silent index corruption.

A more robust solution would be to wait out the source transaction if
CREATE INDEX comes across an INSERT_IN_PROGRESS or DELETE_IN_PROGRESS
HOT tuple. Or you could throw an error, since it's not really likely to
happen (except maybe in a system catalog REINDEX operation). But the
way the patch approaches this at the moment is far too fragile IMHO.

OK. I looked at the code again. In CVS HEAD we assume that we
should never see an DELETE_IN_PROGRESS/INSERT_IN_PROGRESS
unless its deleted/inserted by our own transaction or we are indexing a
system table. Except for these cases, we throw an error.

With HOT, we keep it the same i.e. if we see a
DELETE_IN_PROGRESS/INSERT_IN_PROGRESS tuple, we throw an error,
unless its deleted/inserted by us or this is a system table..
What we change is if we find a DELETE_IN_PROGRESS tuple deleted
by our own transaction and its HOT-updated, we skip that tuple. This is
fine because if the CREATE INDEX commits the DELETE is also committed
and the tuple is not visible (I am still assuming the indcreatxid mechanism
is in place)

So if we don't do HOT update on system tables, the current algorithm
should work fine because we should never find a HOT updated tuple
in the system table and the error-out mechanism should ensure
that we don't corrupt user tables.

So I guess what you are suggesting is to turn on HOT on system tables
and then wait-out any DELETING/INSETING transaction if we find such
a tuple during CREATE INDEX. I think this would work and since we are
talking about system tables, the wait-out business won't be harmful - I
remember there were objections when I suggested this as a general solution.

If we approach it this way, we might also be able to jettison some of

the other complexity such as idxcreatexid.

I doubt, unless we replace it with something better. I think indcreatexid
serves
another purpose. While building an index, we skip any RECENTLY_DEAD
HOT-updated tuples. This is required to handle the existing HOT chains
which are broken with respect to the new index. Essentially what it means
is we don't want to index anything other than the last committed good tuple
in the HOT chain. So when we skip any intermediate RECENTLY_DEAD tuples,
which are potentially visible to the existing running transactions, we want
those transactions NOT to use this new index.

IMHO the extra step in C.I.C simplifies the index build.

If you make the change suggested above, I think you don't need to do
things differently in C.I.C.

I am not sure if I follow you correctly here. The issue with CIC is that
it works with a snapshot. So during initial index build, we might index
a tuple which is in the middle of a broken HOT chain. In the second phase,
we just don't need to index the tuple at the head of the chain, but also
remove the previously inserted tuple, otherwise there would be two
references from the index to the same root heap tuple.

The additional step which does two things:

1) Create catalog entry - stops any new broken HOT chains being created
2) Wait-out existing transactions - makes sure that when the index is built,
we only index the last committed good tuple in the chain.

Since CREATE INDEX works with ShareLock on the relation, we can
safely assume that we can't find any DELETE_IN_PROGRESS tuples except
those deleted by our own transaction. The only exception is system
relation, but we don't do HOT updates on system relation.

Same issue as above: this makes correctness utterly dependent on nobody
releasing locks early.

As I said above, we in fact throw an error for user tables. But if we want
to support HOT updates on system tables, we may need to do the
wait-out business. In fact, now that I think about it there is no other
fundamental reason to not support HOT on system tables. So we
can very well do what you are suggesting.

OK. So if I get you correctly, you are suggesting to acquire cleanup
lock.

If we don't get that, we don't to any maintenance work. Otherwise, we

prune

and repair fragmentation in one go.

Yeah, that's what I'm thinking; then there's no need to track a separate
page state where we've pruned but not defragmented.

I agree. Lets keep it simple and we can always improve it later. The
only thing that worries me how to balance between repair fragmentation
(which is costly operation since it involves several memmoves) and chain
pruning. It might be alright to delay repair operation, but if we end up
with long
chains, fetches might suffer.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On 8/31/07, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

In fact, now that I think about it there is no other
fundamental reason to not support HOT on system tables. So we
can very well do what you are suggesting.

On second thought, I wonder if there is really much to gain by
supporting HOT on system tables and whether it would justify all
the complexity. Initially I thought about CatalogUpdateIndexes to
which we need to teach HOT. Later I also got worried about
building the HOT attribute lists for system tables and handling
all the corner cases for bootstrapping and catalog REINDEX.
It might turn out to be straight forward, but I am not able to
establish that with my limited knowledge in the area.

I would still vote for disabling HOT on catalogs unless you see
strong value in it.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Fri, Aug 31, 2007 at 12:53:51PM +0530, Pavan Deolasee wrote:

On 8/31/07, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:

In fact, now that I think about it there is no other
fundamental reason to not support HOT on system tables. So we
can very well do what you are suggesting.

On second thought, I wonder if there is really much to gain by
supporting HOT on system tables and whether it would justify all
the complexity. Initially I thought about CatalogUpdateIndexes to
which we need to teach HOT. Later I also got worried about
building the HOT attribute lists for system tables and handling
all the corner cases for bootstrapping and catalog REINDEX.
It might turn out to be straight forward, but I am not able to
establish that with my limited knowledge in the area.

I would still vote for disabling HOT on catalogs unless you see
strong value in it.

What about ANALYZE? Doesn't that do a lot of updates?

BTW, I'm 100% in favor of pushing system catalog HOT until later; it's
be silly to risk not getting hot in 8.3 because of catalog HOT.
--
Decibel!, aka Jim Nasby decibel@decibel.org
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)

Decibel! <decibel@decibel.org> writes:

BTW, I'm 100% in favor of pushing system catalog HOT until later; it's
be silly to risk not getting hot in 8.3 because of catalog HOT.

I see this the other way around: if it doesn't work on system catalogs,
it probably doesn't work, period. I'm not in favor of treating the
catalogs differently.

regards, tom lane

On 9/1/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I see this the other way around: if it doesn't work on system catalogs,
it probably doesn't work, period. I'm not in favor of treating the
catalogs differently.

Now that I hear you, I know what to do next :-)

I don't think there is any fundamental problem with system catalogs,
its only the additional complexity that I was worried about. Anyways,
I will rework things as per your suggestion. And I take your point that
making it work on all tables will give us more confidence on the code.

Thanks,
Pavan

P.S. Next week is bad for me :-( I am on vacation on Thursday/Friday
and for remaining days, I may not be able to spend extra cycles,
apart from regular working hours.

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com