SEO

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> Please see the revised patches attached.

I'm curious how much the WAL-recovery aspects of this patch have been
tested, because heap_xlog_clean seems quite broken. You have apparently
decided to redefine the WAL record format as using one-based rather than
zero-based item offsets, which would be fine if any of the rest of the
code had been changed to agree ...

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

On 9/13/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
> I'm curious how much the WAL-recovery aspects of this patch have been
> tested, because heap_xlog_clean seems quite broken.



There are quite a few crash recovery tests that one of our QA persons
(Dharmendra Goyal) has written. I can post them if necessary. We run
these tests very regularly.

Apart from these regression crash tests, I had mostly tested by
running lot of concurrent clients on UP/SMP machines, crashing
and recovering the database. We fixed quite a few issues with
these tests. I have tried crashing in middle of UPDATEs/INSERTs/DELETEs
and VACUUM/VACUUM FULL.


You have apparently
> decided to redefine the WAL record format as using one-based rather than
> zero-based item offsets, which would be fine if any of the rest of the
> code had been changed to agree ...
>
>
I know Heikki changed that when he did the initial refactoring, but not
sure why. May be he wanted to make it more consistent.
But I don't think its broken because we collect the offsets in one-based
format in PageRepairFragmentation, WAL log in that format and redo
the same way. Am I missing something ?

Thanks,
Pavan

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

Pavan Deolasee wrote:
> On 9/13/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> You have apparently
>> decided to redefine the WAL record format as using one-based rather than
>> zero-based item offsets, which would be fine if any of the rest of the
>> code had been changed to agree ...
>>
>>
> I know Heikki changed that when he did the initial refactoring, but not
> sure why. May be he wanted to make it more consistent.

Yeah, I just checked the work-in-progress patch I sent you back in July.
I refactored it to use one-based offsets for consistency, since I
modified log_heap_clean quite heavily anyway.

It's possible that I broke it in the process, I was only interested in
testing the performance characteristics of the simplified pruning scheme.

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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

On 9/13/07, Heikki Linnakangas <heikki@enterprisedb.com> wrote:
>
>
>
> Yeah, I just checked the work-in-progress patch I sent you back in July.
> I refactored it to use one-based offsets for consistency, since I
> modified log_heap_clean quite heavily anyway.
>
> It's possible that I broke it in the process, I was only interested in
> testing the performance characteristics of the simplified pruning scheme.
>
>
I don't think so -- atleast I couldn't figure out why its broken. But I
would take Tom's comment seriously and look more into it tomorrow.
Or we can just revert it back to zero-based offsets.

Thanks,
Pavan

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

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> On 9/13/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> You have apparently
>> decided to redefine the WAL record format as using one-based rather than
>> zero-based item offsets, which would be fine if any of the rest of the
>> code had been changed to agree ...
>>
> I know Heikki changed that when he did the initial refactoring, but not
> sure why. May be he wanted to make it more consistent.
> But I don't think its broken because we collect the offsets in one-based
> format in PageRepairFragmentation, WAL log in that format and redo
> the same way. Am I missing something ?

Hmm, I had been thinking that vacuum.c and vacuumlazy.c worked directly
with that info, but it looks like you're right, only
PageRepairFragmentation touches that array. Never mind ... though my
suspicions would probably not have been aroused if anyone had bothered
to fix the comments.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

On 9/13/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
>
> Never mind ... though my
> suspicions would probably not have been aroused if anyone had bothered
> to fix the comments.
>
>
Yeah, my fault. I should have fixed that. Sorry about that.


Thanks,
Pavan


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

Okay, something else (a real problem this time ;-)):
HeapTupleSatisfiesAbort is bogus because it has failed to track recent
changes in tqual.c.

Rather than fix it, though, I question why we need it at all. The only
use of it is in heap_prune_tuplechain() and ISTM that code is redundant,
wrong, or both.

In the case of a full-page prune, it's redundant because the tuple would
be found anyway by searching its chain from the root tuple. Indeed I
suspect that such a tuple is entered into the newly-dead list twice,
perhaps risking overflow of that array.

In the case of a single-chain prune, it still seems wrong since you'll
eliminate only one of what might be a chain with multiple aborted
entries. If it's OK to leave those other entries for future collection,
then it should be OK to leave this one too. If it's not OK then the
approach needs to be redesigned.

I'm fairly unclear on what the design intention is for recovering from
the case where the last item(s) on a HOT chain are aborted. Please
clarify.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

On 9/14/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
> HeapTupleSatisfiesAbort is bogus because it has failed to track recent
> changes in tqual.c.



Oh. I should have been aware.


Rather than fix it, though, I question why we need it at all. The only
> use of it is in heap_prune_tuplechain() and ISTM that code is redundant,
> wrong, or both.
>
> In the case of a full-page prune, it's redundant because the tuple would
> be found anyway by searching its chain from the root tuple. Indeed I
> suspect that such a tuple is entered into the newly-dead list twice,
> perhaps risking overflow of that array.



No, we would never reach an aborted dead tuple from the chain because
we would see a live tuple before that. Or the tuple preceding the aborted
(first aborted, if there are many) may have been updated again and the
chain never reaches to the aborted tuples. Thats the reason why we have
HeapTupleSatisfiesAbort to collect any aborted heap-only tuples.



In the case of a single-chain prune, it still seems wrong since you'll
> eliminate only one of what might be a chain with multiple aborted
> entries. If it's OK to leave those other entries for future collection,
> then it should be OK to leave this one too. If it's not OK then the
> approach needs to be redesigned.



Again, since we check every heap-only tuple for aborted dead
case we shall collect all such tuples. I think one place where you
are confusing (or IOW the code might have confused you ;-))
is that we never reach aborted dead heap-only tuples by
following the HOT chain from the root tuple and thats why it
needs special treatment.

I'm fairly unclear on what the design intention is for recovering from
> the case where the last item(s) on a HOT chain are aborted. Please
> clarify.
>
>
We don't do anything special. Such a tuple is never reached during
HOT chain following because either we would see a visible tuple before
that or the older tuple might have been updated again and the chain
had moved in a different direction. We only need some special treatment
to prune such tuple and hence the business of HeapTupleSatisfiesAbort

Having said that, based on our recent discussion about pruning a chain
upto and including the latest DEAD tuple in the chain, ISTM that we can
get rid of the giving any special treatment to aborted heap-only
tuples. Earlier we could not do so for "HOT updated aborted heap-only"
because we did not know if its a part of any valid HOT chain. Now
(assuming we change the pruning code to always prune everything including
the latest DEAD tuple) we guarantee that all DEAD tuples in the chain will
be pruned, and hence we can collect any DEAD tuple seen while pruning.

I am not sure if I explain this well, may be I should post an example.
Need to run now.

Thanks,
Pavan

--

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