On Thu, Mar 23, 2023 at 3:06 PM Robert Haas wrote:
> On Thu, Mar 23, 2023 at 1:34 PM Robert Haas wrote:
> > OK, let me spend some more time on this and I'll post a patch (or
> > patches) in a bit.
>
> All right, here are some more fixups.
It looks like e88754a1965c0f40a723e6e46d670cacda9e19bd
On Thu, Mar 23, 2023 at 4:36 PM Andres Freund wrote:
> Could it be that the tests didn't exercise the path before?
Hmm, perhaps.
> > Nonetheless, here's a patch. I notice that there's a similar problem
> > in another place, too. get_xid_status() is called a total of five
> > times and it looks
Hi,
On 2023-03-23 15:37:15 -0400, Robert Haas wrote:
> On Wed, Mar 22, 2023 at 8:38 PM Andres Freund wrote:
> > skink / valgrind reported in a while back and found another issue:
> >
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink=2023-03-22%2021%3A53%3A41
> >
> > ==2490364==
On Wed, Mar 22, 2023 at 8:38 PM Andres Freund wrote:
> skink / valgrind reported in a while back and found another issue:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink=2023-03-22%2021%3A53%3A41
>
> ==2490364== VALGRINDERROR-BEGIN
> ==2490364== Conditional jump or move depends
On Thu, Mar 23, 2023 at 1:34 PM Robert Haas wrote:
> OK, let me spend some more time on this and I'll post a patch (or
> patches) in a bit.
All right, here are some more fixups.
--
Robert Haas
EDB: http://www.enterprisedb.com
0001-amcheck-Tighten-up-validation-of-redirect-line-point.patch
Hi,
On 2023-03-23 11:20:04 +0530, Himanshu Upadhyaya wrote:
> On Thu, Mar 23, 2023 at 2:15 AM Andres Freund wrote:
>
> >
> > Currently the new verify_heapam() follows ctid chains when XMAX_INVALID is
> > set
> > and expects to find an item it can dereference - but I don't think that's
> >
On Thu, Mar 23, 2023 at 1:26 PM Andres Freund wrote:
> E.g. continuing after:
>
> rditem = PageGetItemId(ctx.page, rdoffnum);
> if (!ItemIdIsUsed(rditem))
> report_corruption(,
>
Hi,
On 2023-03-23 11:41:52 -0400, Robert Haas wrote:
> On Wed, Mar 22, 2023 at 5:56 PM Andres Freund wrote:
> > I also think it's not quite right that some of checks inside if
> > (ItemIdIsRedirected()) continue in case of corruption, others don't. While
> > there's a later continue, that means
On Thu, Mar 23, 2023 at 9:42 AM Robert Haas wrote:
> Hmph. I didn't think very hard about that code and just assumed
> Himanshu had tested it. I don't have convenient access to a Big-endian
> test machine myself. Are you able to check whether using 0x00010019
> unconditionally works?
Oh, I see
On Wed, Mar 22, 2023 at 5:42 PM Peter Geoghegan wrote:
> However, this "second pass over page" loop has roughly the same
> problem as the nearby HeapTupleHeaderIsHotUpdated() coding pattern: it
> doesn't account for the fact that a tuple whose xmin was
> XID_IN_PROGRESS a little earlier on may
On Wed, Mar 22, 2023 at 5:56 PM Andres Freund wrote:
> Why are redirections now checked in two places? There already was a
> ItemIdIsUsed() check in the "/* Perform tuple checks */" loop, but now there's
> the ItemIdIsRedirected() check in the "Update chain validation." loop as well
> - and the
On Wed, Mar 22, 2023 at 4:45 PM Andres Freund wrote:
> At the very least there's missing verification that tids actually exists in
> the
> "Update chain validation" loop, leading to:
> TRAP: failed Assert("ItemIdHasStorage(itemId)"), File:
>
On Wed, Mar 22, 2023 at 3:27 PM Tom Lane wrote:
> My animal mamba doesn't like this one bit.
>
> I suspect the reason is that it's big-endian (PPC) and the endianness
> hacking in the test is simply wrong:
>
> syswrite($file,
> pack("L", $ENDIANNESS eq 'little' ?
On Thu, Mar 23, 2023 at 2:15 AM Andres Freund wrote:
>
> Currently the new verify_heapam() follows ctid chains when XMAX_INVALID is
> set
> and expects to find an item it can dereference - but I don't think that's
> something we can rely on: Afaics HOT pruning can break chains, but doesn't
>
Hi,
On 2023-03-22 13:45:52 -0700, Andres Freund wrote:
> On 2023-03-22 09:19:18 -0400, Robert Haas wrote:
> > On Fri, Mar 17, 2023 at 8:31 AM Aleksander Alekseev
> > wrote:
> > > The patch needed a rebase due to a4f23f9b. PFA v12.
> >
> > I have committed this after tidying up a bunch of things
Hi,
On 2023-03-22 14:56:22 -0700, Andres Freund wrote:
> A patch addressing some, but not all, of those is attached. With that I don't
> see any crashes or false-positives anymore.
That patch missed that, as committed, the first if (ItemIdIsRedirected())
check sets lp_valid[n] = true even if the
Hi,
On 2023-03-22 13:45:52 -0700, Andres Freund wrote:
> On 2023-03-22 09:19:18 -0400, Robert Haas wrote:
> > On Fri, Mar 17, 2023 at 8:31 AM Aleksander Alekseev
> > wrote:
> > > The patch needed a rebase due to a4f23f9b. PFA v12.
> >
> > I have committed this after tidying up a bunch of things
On Wed, Mar 22, 2023 at 2:14 PM Peter Geoghegan wrote:
> > Currently the new verify_heapam() follows ctid chains when XMAX_INVALID is
> > set
> > and expects to find an item it can dereference - but I don't think that's
> > something we can rely on: Afaics HOT pruning can break chains, but
On Wed, Mar 22, 2023 at 1:45 PM Andres Freund wrote:
> At the very least there's missing verification that tids actually exists in
> the
> "Update chain validation" loop, leading to:
> TRAP: failed Assert("ItemIdHasStorage(itemId)"), File:
>
Hi,
On 2023-03-22 09:19:18 -0400, Robert Haas wrote:
> On Fri, Mar 17, 2023 at 8:31 AM Aleksander Alekseev
> wrote:
> > The patch needed a rebase due to a4f23f9b. PFA v12.
>
> I have committed this after tidying up a bunch of things in the test
> case file that I found too difficult to
Robert Haas writes:
> I have committed this after tidying up a bunch of things in the test
> case file that I found too difficult to understand -- or in some cases
> just incorrect, like:
My animal mamba doesn't like this one bit.
I suspect the reason is that it's big-endian (PPC) and the
On Fri, Mar 17, 2023 at 8:31 AM Aleksander Alekseev
wrote:
> The patch needed a rebase due to a4f23f9b. PFA v12.
I have committed this after tidying up a bunch of things in the test
case file that I found too difficult to understand -- or in some cases
just incorrect, like:
elsif ($offnum
Hi,
> On Wed, Mar 8, 2023 at 7:30 PM Himanshu Upadhyaya
> wrote:
> Please find the v11 patch with all these changes.
The patch needed a rebase due to a4f23f9b. PFA v12.
--
Best regards,
Aleksander Alekseev
v12-0001-Implement-HOT-chain-validation-in-verify_heapam.patch
Description: Binary
1
From: Himanshu Upadhyaya
Date: Thu, 9 Mar 2023 21:18:58 +0530
Subject: [PATCH v11] Implement HOT chain validation in verify_heapam()
Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev, Andres Freund.
Some revisions by Robert Haas.
---
contrib/amcheck/verify_heapam.c
On Wed, Mar 8, 2023 at 7:06 PM Robert Haas wrote:
>
> > +/* HOT chains should not intersect. */
> > +if (predecessor[nextoffnum] != InvalidOffsetNumber)
> > +{
> > +report_corruption(,
> > +
Hi,
> > ```
> > +$node->append_conf('postgresql.conf','max_prepared_transactions=100');
> > ```
> >
> > From what I can tell this line is not needed.
>
> That surprises me, because the new test cases involve preparing a
> transaction, and by default max_prepared_transactions=0. So it seems
> to
On Wed, Mar 8, 2023 at 5:35 AM Aleksander Alekseev
wrote:
> I did, both with Meson and Autotools. All in all the patch looks very
> good, but I have a few little nitpicks.
Thank you for the nitpicks.
> +/* HOT chains should not intersect. */
> +if
Hi,
> Note that I have not tried any of this yet.
I did, both with Meson and Autotools. All in all the patch looks very
good, but I have a few little nitpicks.
```
+/* HOT chains should not intersect. */
+if (predecessor[nextoffnum] != InvalidOffsetNumber)
+
> On Mar 7, 2023, at 10:16 AM, Robert Haas wrote:
>
> On Mon, Mar 6, 2023 at 12:36 PM Robert Haas wrote:
>> So it seems that we still don't have a patch where the
>> value of a variable called lp_valid corresponds to whether or not the
>> L.P. is valid.
>
> Here's a worked-over version of
On Mon, Mar 6, 2023 at 12:36 PM Robert Haas wrote:
> So it seems that we still don't have a patch where the
> value of a variable called lp_valid corresponds to whether or not the
> L.P. is valid.
Here's a worked-over version of this patch. Changes:
- I got rid of the code that sets lp_valid in
On Thu, Feb 9, 2023 at 12:09 PM Himanshu Upadhyaya
wrote:
> Initially while implementing logic to identify the root of the HOT chain
> I was getting crash and regression failure's that time I thought of having
> this check along with a few other changes that were required,
> but you are right,
ds,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 19c93cce0189150d6bfe68237eb3d5a414a18ad9 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya
Date: Thu, 9 Feb 2023 22:00:25 +0530
Subject: [PATCH v10] Implement HOT chain validation in verify_heapam()
Himanshu Upadhyaya, reviewe
On Sun, Feb 5, 2023 at 3:57 AM Himanshu Upadhyaya
wrote:
> Thanks, yes it's working fine with Prepared Transaction.
> Please find attached the v9 patch incorporating all the review comments.
I don't know quite how we're still going around in circles about this,
but this code makes no sense to me
ached the v9 patch incorporating all the review comments.
--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From d241937841c5990ff4df00d1abc6bfbb3491ac3e Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya
Date: Sun, 5 Feb 2023 12:50:21 +0530
Subject: [PATCH v9] Implement H
On Mon, Jan 30, 2023 at 8:24 AM Himanshu Upadhyaya
wrote:
> Before this we stop the node by "$node->stop;" and then only we progress to
> manual corruption. This will abort all running/in-progress transactions.
> So, if we create an in-progress transaction and comment "$node->stop;"
> then
Hi Hackers,
On Sun, Jan 22, 2023 at 8:48 PM Himanshu Upadhyaya <
upadhyaya.himan...@gmail.com> wrote:
>
> The test if (pred_in_progress || TransactionIdDidCommit(curr_xmin))
>> seems wrong to me. Shouldn't it be &&? Has this code been tested at
>> all? It doesn't seem to have a test case. Some
On Sun, Jan 22, 2023 at 10:19 AM Himanshu Upadhyaya
wrote:
> I was trying to use lp_valid as I need to identify the root of the HOT chain
> and we are doing validation on the root of the HOT chain when we loop over
> the predecessor array.
> Was resetting lp_valid in the last patch because we
On Fri, Jan 20, 2023 at 12:38 AM Robert Haas wrote:
>
> I think that the handling of lp_valid[] in the loop that begins with
> "Loop over offset and populate predecessor array from all entries that
> are present in successor array" is very confusing. I think that
> lp_valid[] should be answering
On Thu, Jan 19, 2023 at 8:55 AM Aleksander Alekseev
wrote:
> I noticed that this patch stuck a little and decided to take another look.
>
> It seems to be well written, covered with tests and my understanding
> is that all the previous feedback was accounted for. To your knowledge
> is there
Hi hackers,
> Fixed.
I noticed that this patch stuck a little and decided to take another look.
It seems to be well written, covered with tests and my understanding
is that all the previous feedback was accounted for. To your knowledge
is there anything that prevents us from moving it to "Ready
e' due to some check.
--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 02f0d19bd82f390df8e3d732d60cf3eb0ae1dc97 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya
Date: Mon, 5 Dec 2022 18:28:33 +0530
Subject: [PATCH v8] Implement HOT chain validation in verify_he
Hi,
On Fri, Dec 2, 2022 at 5:43 AM Andres Freund wrote:
>
> > + curr_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> curr_lp);
> > + curr_xmin = HeapTupleHeaderGetXmin(curr_htup);
> > + xid_status = get_xid_status(curr_xmin, ,
>
Hi,
On 2022-11-30 16:09:19 +0530, Himanshu Upadhyaya wrote:
> has been updated to handle cases of sub-transaction.
Thanks!
> + /* Loop over offsets and validate the data in the predecessor
> array. */
> + for (OffsetNumber currentoffnum = FirstOffsetNumber;
>
r's xmin is aborted or in
> progress, the
> > + * current tuples xmin should be aborted or in
> progress
> > + * respectively. Also both xmin's must be equal.
> > + */
> > + if (!Transactio
On Mon, Nov 21, 2022 at 1:34 PM Andres Freund wrote:
> Hm. But to get to that point we already need to have decided that xmax
> is not a normal xid. Unhelpfully we reuse the 'xid' variable for xmax as
> well:
> xid = HeapTupleHeaderGetRawXmax(tuple);
>
> I don't really know the
Hi,
On 2022-11-20 11:58:12 -0800, Peter Geoghegan wrote:
> There is code in heap_prepare_freeze_tuple() that treats a raw xmax as
> "xmax_already_frozen = true", even when the raw xmax value isn't
> already set to InvalidTransactionId. I'm referring to this code:
>
> if ( ... ) // process raw
On Tue, Nov 15, 2022 at 10:55 PM Andres Freund wrote:
> I'm quite certain that it's possible to end up freezing an earlier row
> versions in a hot chain in < 14, I got there with careful gdb
> orchestration. Of course possible I screwed something up, given I did it once,
> interactively. Not sure
Hi,
On 2022-11-17 21:33:17 +0530, Himanshu Upadhyaya wrote:
> On Tue, Nov 15, 2022 at 3:32 AM Andres Freund wrote:
> > > Furthermore, it is
> > > possible that successor[x] = successor[x'] since the page might be
> > corrupted
> > > and we haven't checked otherwise.
> > >
> > > predecessor[y] =
On Tue, Nov 15, 2022 at 3:32 AM Andres Freund wrote:
>
> > Furthermore, it is
> > possible that successor[x] = successor[x'] since the page might be
> corrupted
> > and we haven't checked otherwise.
> >
> > predecessor[y] = x means that successor[x] = y but in addition we've
> > checked that y
On Wed, Nov 16, 2022 at 12:41 PM Himanshu Upadhyaya <
upadhyaya.himan...@gmail.com> wrote:
>
>
>> > + }
>> > +
>> > + /* Loop over offsets and validate the data in the
>> predecessor array. */
>> > + for (OffsetNumber currentoffnum = FirstOffsetNumber;
>>
On Wed, Nov 16, 2022 at 10:57 PM Himanshu Upadhyaya
wrote:
> I think if pred_xmin is aborted and curr_xmin is in progress we should
> consider it as a corruption case but vice versa is not true.
Yeah, you're right. I'm being stupid about subtransactions again.
--
Robert Haas
EDB:
On Wed, Nov 16, 2022 at 11:23 PM Robert Haas wrote:
> On Wed, Nov 16, 2022 at 4:51 AM Himanshu Upadhyaya
> wrote:
> > yes, got it, have tried to test and it is giving false corruption in
> case of subtransaction.
> > I think a better way to have this check is, we need to check that if
>
On Wed, Nov 16, 2022 at 4:51 AM Himanshu Upadhyaya
wrote:
> yes, got it, have tried to test and it is giving false corruption in case of
> subtransaction.
> I think a better way to have this check is, we need to check that if
> pred_xmin is
> aborted then current_xmin should be aborted only. So
On Wed, Nov 16, 2022 at 1:58 AM Robert Haas wrote:
> On Tue, Nov 15, 2022 at 2:50 PM Andres Freund wrote:
> > On 2022-11-15 11:36:21 -0500, Robert Haas wrote:
> > > On Mon, Nov 14, 2022 at 5:02 PM Andres Freund
> wrote:
> > > > It seems like we should do a bit more validation within a chain of
On Thu, Nov 10, 2022 at 3:38 AM Andres Freund wrote:
>
> > + }
> > +
> > + /*
> > + * Loop over offset and populate predecessor array from
> all entries
> > + * that are present in successor array.
> > + */
> > +
Hi,
On 2022-11-14 15:07:05 -0800, Peter Geoghegan wrote:
> I'd really like to know if the scary HOT chain freezing scenario is
> possible, for the very obvious reason. Have you tried to write a test
> case for that?
I tried. Unfortunately, even if the bug exists, we currently don't have the
On Tue, Nov 15, 2022 at 2:50 PM Andres Freund wrote:
> On 2022-11-15 11:36:21 -0500, Robert Haas wrote:
> > On Mon, Nov 14, 2022 at 5:02 PM Andres Freund wrote:
> > > It seems like we should do a bit more validation within a chain of
> > > tuples. E.g. that no live tuple can follow an !DidCommit
Hi,
On 2022-11-15 11:36:21 -0500, Robert Haas wrote:
> On Mon, Nov 14, 2022 at 5:02 PM Andres Freund wrote:
> > It seems like we should do a bit more validation within a chain of
> > tuples. E.g. that no live tuple can follow an !DidCommit xmin?
>
> I think this check is already present in
On Mon, Nov 14, 2022 at 5:02 PM Andres Freund wrote:
> On 2022-11-14 14:27:54 -0500, Robert Haas wrote:
> > On Wed, Nov 9, 2022 at 5:08 PM Andres Freund wrote:
> > > I don't really understand this logic - why can't we populate the
> > > predecessor
> > > array, if we can construct a successor
On Mon, Nov 14, 2022 at 2:58 PM Andres Freund wrote:
> On 2022-11-14 14:42:16 -0800, Peter Geoghegan wrote:
> > What does this have to tell us, if anything, about the implications
> > for code on HEAD?
>
> Nothing really test I sent (*) - I wanted to advance the discussion about the
> patch being
Hi,
On 2022-11-14 14:42:16 -0800, Peter Geoghegan wrote:
> What does this have to tell us, if anything, about the implications
> for code on HEAD?
Nothing really test I sent (*) - I wanted to advance the discussion about the
patch being wrong as-is in a concrete way.
This logic was one of my
On Mon, Nov 14, 2022 at 2:33 PM Andres Freund wrote:
> > Why don't you think that there is corruption?
>
> I looked at the state after the test and the complaint is bogus. It's caused
> by the patch ignoring the cur->xmax == next->xmin condition if next->xmin is
> FrozenTransactionId. The
Hi,
On 2022-11-14 14:13:10 -0800, Peter Geoghegan wrote:
> > I think the problem partially is that the proposed verify_heapam() code is
> > too
> > "aggressive" considering things to be part of the same hot chain - which
> > then
> > means we have to be very careful about erroring out.
> >
> >
On Mon, Nov 14, 2022 at 12:58 PM Andres Freund wrote:
> I wonder if we ought to add an error check to heap_prepare_freeze_tuple()
> against this scenario. We're working towards being more aggressive around
> freezing, which will make it more likely to hit corner cases around this.
In theory my
Hi,
On 2022-11-14 14:27:54 -0500, Robert Haas wrote:
> On Wed, Nov 9, 2022 at 5:08 PM Andres Freund wrote:
> > I don't really understand this logic - why can't we populate the predecessor
> > array, if we can construct a successor entry?
>
> This whole thing was my idea, so let me try to
On Mon, Nov 14, 2022 at 11:28 AM Robert Haas wrote:
> Part of the motivation here is also driven by trying to figure out how
> to word the complaints. We have a dedicated field in the amcheck that
> can hold one tuple offset or the other, but if we're checking the
> relationships between tuples,
Hi,
On 2022-11-14 09:50:48 -0800, Peter Geoghegan wrote:
> On Mon, Nov 14, 2022 at 9:38 AM Andres Freund wrote:
> > Anyway, I played a bit around with this. It's hard to hit, not because we
> > somehow won't choose such a horizon, but because we'll commonly prune the
> > earlier tuple version
On Wed, Nov 9, 2022 at 5:08 PM Andres Freund wrote:
> I don't really understand this logic - why can't we populate the predecessor
> array, if we can construct a successor entry?
This whole thing was my idea, so let me try to explain. I think the
naming and comments need work, but I believe the
On Mon, Nov 14, 2022 at 9:38 AM Andres Freund wrote:
> Anyway, I played a bit around with this. It's hard to hit, not because we
> somehow won't choose such a horizon, but because we'll commonly prune the
> earlier tuple version away due to xmax being old enough.
That must be a bug, then. Since,
Hi,
On 2022-11-09 18:35:12 -0800, Peter Geoghegan wrote:
> On Wed, Nov 9, 2022 at 6:10 PM Andres Freund wrote:
> > And thinking about it, it'd be quite bad if the horizon worked that way.
> > You can easily construct a workload where every single xid would "skewer"
> > some chain, never
On Wed, Nov 9, 2022 at 6:10 PM Andres Freund wrote:
> And thinking about it, it'd be quite bad if the horizon worked that way. You
> can easily construct a workload where every single xid would "skewer" some
> chain, never allowing the horizon to be raised.
Your whole scenario is one involving
On Wed, Nov 9, 2022 at 5:46 PM Andres Freund wrote:
> > Putting all 3 together: doesn't it seem quite likely that the way that
> > we compute OldestXmin is the factor that prevents "skewering" of an
> > update chain? What else could possibly be preventing corruption here?
> > (Theoretically it
Hi,
And thinking about it, it'd be quite bad if the horizon worked that way. You
can easily construct a workload where every single xid would "skewer" some
chain, never allowing the horizon to be raised.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi,
On 2022-11-09 17:32:46 -0800, Peter Geoghegan wrote:
> > The xmin horizon is very coarse grained. Just because it is 7 doesn't mean
> > that xid 10 is still running. All it means that one backend or slot has an
> > xmin or xid of 7.
>
> Of course that's true. But I wasn't talking about the
On Wed, Nov 9, 2022 at 4:15 PM Andres Freund wrote:
> To me this is extending the problem into more areas rather than reducing
> it. I'd have *zero* confidence in any warnings that amcheck issued that
> involved <9.4 special cases.
Maybe you would at first. But then we get to learn what mistake
Hi,
On 2022-11-09 15:03:39 -0800, Peter Geoghegan wrote:
> > > + /*
> > > + * Add a line pointer offset to the predecessor
> > > array if xmax is
> > > + * matching with xmin of next tuple (reaching via
> > > its t_ctid).
> > > +
On Wed, Nov 9, 2022 at 2:08 PM Andres Freund wrote:
> To start with: I think this is an extremely helpful and important
> feature. Both for checking production systems and for finding problems during
> development.
+1.
It's painful to get this in, in part because we now have to actually
decide
17:44:56 +0530
> Subject: [PATCH v6] Implement HOT chain validation in verify_heapam()
>
> Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev
>
> Discussion:
> https://postgr.es/m/CAPF61jBBR2-iE-EmN_9v0hcQEfyz_17e5Lbb0%2Bu2%3D9ukA9sWmQ%40mail.gmail.c
Hi Himanshu,
> Test cases are now part of this v6 patch.
I believe the patch is in pretty good shape now. I'm going to change
its status to "Ready for Committer" soon unless there are going to be
any objections.
--
Best regards,
Aleksander Alekseev
the previous tuple's state first.
>
> Done.
> +if (!TransactionIdIsValid(curr_xmax) &&
> HeapTupleHeaderIsHotUpdated(tuphdr))
> +{
> +report_corruption(ctx,
> + psprintf("tuple has been updated, but xmax is
> 0")
On Tue, Sep 27, 2022 at 1:35 AM Robert Haas wrote:
> On Sat, Sep 24, 2022 at 8:45 AM Himanshu Upadhyaya
> wrote:
> > Here our objective is to validate if both Predecessor's xmin and current
> Tuple's xmin are same then cmin of predecessor must be less than current
> Tuple's cmin. In case when
On Sat, Sep 24, 2022 at 8:45 AM Himanshu Upadhyaya
wrote:
> Here our objective is to validate if both Predecessor's xmin and current
> Tuple's xmin are same then cmin of predecessor must be less than current
> Tuple's cmin. In case when both tuple xmin's are same then I think
> predecessor's
On Tue, Sep 20, 2022 at 6:43 PM Robert Haas wrote:
> I disapprove of ignoring the HEAP_COMBOCID flag. Emitting a message
> claiming that the CID has a certain value when that's actually a combo
> CID is misleading, so at least a different message wording is needed
> in such cases. But it's also
On Tue, Sep 20, 2022 at 5:00 AM Himanshu Upadhyaya
wrote:
> Please find it attached.
This patch still has no test cases. Just as we have test cases for the
existing corruption checks, we should have test cases for these new
corruption checks, showing cases where they actually fire.
I think I
Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya
Date: Tue, 20 Sep 2022 14:23:31 +0530
Subject: [PATCH v5] Implement HOT chain validation in verify_heapam()
Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev
Discussion: https://postgr.es/m/CAPF61jBBR2-iE-EmN_9v0hcQEfyz_17e
Hi Himanshu,
> I have changed this in the attached patch.
If it's not too much trouble could you please base your changes on v4
that I submitted? I put some effort into writing a proper commit
message, editing the comments, etc. The easiest way of doing this is
using `git am` and `git
egards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From 1a51c544c5c9a14441831f5299b9d6fe275fbf53 Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya
Date: Mon, 19 Sep 2022 21:44:34 +0530
Subject: [PATCH v4] HOT chain validation in verify_heapam
---
contrib/amcheck/verif
Hi Himanshu,
> Done, updated in the v3 patch.
Thanks for the updated patch.
Here is v4 with fixed compiler warnings and some minor tweaks from me.
I didn't put too much thought into the algorithm but I already see
something strange. At verify_heapam.c:553 you declared curr_xmax and
next_xmin.
On Wed, Sep 7, 2022 at 12:11 AM Robert Haas wrote:
>
> I think the check should be written like this:
>
> !TransactionIdEquals(pred_xmin, curr_xmin) &&
> !TransctionIdDidCommit(pred_xmin)
>
> The TransactionIdEquals check should be done first for the reason you
> state: it's cheaper.
>
> I think
4a32d7cbdec0d090e99a9a58cb4deb5cb4c03aef Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya
Date: Mon, 19 Sep 2022 13:50:47 +0530
Subject: [PATCH v3] HOT chain validation in verify_heapam
---
contrib/amcheck/verify_heapam.c | 209
1 file changed, 209 insertio
On Fri, Sep 9, 2022 at 10:00 AM Himanshu Upadhyaya
wrote:
> Approach of introducing a successor array is good but I see one overhead with
> having both successor and predecessor array, that is, we will traverse each
> offset on page thrice(one more for original loop on offset) and with each
>
On Wed, Sep 7, 2022 at 2:49 AM Robert Haas wrote:
>
> But here's one random idea: add a successor[] array and an lp_valid[]
> array. In the first loop, set lp_valid[offset] = true if it passes the
> check_lp() checks, and set successor[A] = B if A redirects to B or has
> a CTID link to B,
On Tue, Sep 6, 2022 at 6:34 AM Himanshu Upadhyaya
wrote:
>> This isn't safe because the line pointer referenced by rditem may not
>> have been sanity-checked yet. Refer to the comment just below where it
>> says "Sanity-check the line pointer's offset and length values".
>>
> handled by creating
On Tue, Sep 6, 2022 at 9:38 AM Aleksander Alekseev
wrote:
> > Please correct me if I'm wrong, but don't we have a race condition here:
> >
> > ```
> > +if ((TransactionIdDidAbort(pred_xmin) ||
> > TransactionIdIsInProgress(pred_xmin))
> > +&&
Hi hackers,
> Please correct me if I'm wrong, but don't we have a race condition here:
>
> ```
> +if ((TransactionIdDidAbort(pred_xmin) ||
> TransactionIdIsInProgress(pred_xmin))
> +&& !TransactionIdEquals(pred_xmin, curr_xmin))
> {
> ```
>
> The scenario
Hi again,
> I am looking into the process of adding the TAP test for these changes and
> finding a way to corrupt a page in the tap test
Please note that currently the patch breaks many existing tests. I
suggest fixing these first.
For the details please see the cfbot report [1] or execute the
Hi Himanshu,
Many thanks for working on this!
> Please find attached the patch with the above idea of HOT chain's validation
Please correct me if I'm wrong, but don't we have a race condition here:
```
+if ((TransactionIdDidAbort(pred_xmin) ||
TransactionIdIsInProgress(pred_xmin))
psprintf("Current tuple at offset %u is
> HOT but it is next updated tuple of last Tuple in HOT chain.",
> +(unsigned) ctx.offnum));
> +}
>
> Three if-statements up, you tested two out of these three conditions
> and c
Hi,
Thanks for working on this.
+htup = (HeapTupleHeader) PageGetItem(ctx.page, rditem);
+if (!(HeapTupleHeaderIsHeapOnly(htup) &&
htup->t_infomask & HEAP_UPDATED))
+report_corruption(,
+
on of the patch.
--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
From f3ae2f783a255109655cade770ebc74e01aef34c Mon Sep 17 00:00:00 2001
From: Himanshu Upadhyaya
Date: Thu, 18 Aug 2022 13:20:51 +0530
Subject: [PATCH v1] HOT chain validation in verify_heapam.
---
100 matches
Mail list logo
