From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Reviewing freeze map code |
Date: | 2016-06-10 02:33:52 |
Message-ID: | 20160610023349.lg3fafqtv2aavqup@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
I found two, relatively minor, issues.
1) I think we should perform a relkind check in
collect_corrupt_items(). Atm we'll "gladly" run against an index. If
we actually entered the main portion of the loop in
collect_corrupt_items(), that could end up corrupting the table (via
HeapTupleSatisfiesVacuum()). But it's probably safe, because the vm
fork doesn't exist for anything but heap/toast relations.
2) GetOldestXmin() currently specifies a relation, which can cause
trouble in recovery:
/*
* If we're not computing a relation specific limit, or if a shared
* relation has been passed in, backends in all databases have to be
* considered.
*/
allDbs = rel == NULL || rel->rd_rel->relisshared;
/* Cannot look for individual databases during recovery */
Assert(allDbs || !RecoveryInProgress());
I think that needs to be fixed.
3) Harmless here, but I think it's bad policy to release locks
on normal relations before the end of xact.
+ relation_close(rel, AccessShareLock);
+
i.e. we'll Assert out.
4)
+ if (check_visible)
+ {
+ HTSV_Result state;
+
+ state = HeapTupleSatisfiesVacuum(&tuple, OldestXmin, buffer);
+ if (state != HEAPTUPLE_LIVE ||
+ !HeapTupleHeaderXminCommitted(tuple.t_data))
+ record_corrupt_item(items, &tuple.t_data->t_ctid);
+ else
This theoretically could give false positives, if GetOldestXmin() went
backwards. But I think that's ok.
5) There's a bunch of whitespace damage in the diff, like
Oid relid = PG_GETARG_OID(0);
- MemoryContext oldcontext;
+ MemoryContext oldcontext;
Otherwise this looks good. I played with it for a while, and besides
finding intentionally caused corruption, it didn't flag anything
(besides crashing on a standby, as in 2)).
Greetings,
Andres Freund
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2016-06-10 02:38:35 | Re: Reviewing freeze map code |
Previous Message | Haribabu Kommi | 2016-06-10 02:23:54 | Re: WIP: Data at rest encryption |