WAL consistency check facility

On Mon, Aug 22, 2016 at 9:44 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:

Please let me know your thoughts on this.

Since custom AMs have been introduced, I have kept that in a corner of
my mind and thought about it a bit. And while the goal of this patch
is clearly worth it, I don't think that the page masking interface is
clear at all. For example, your patch completely ignores
contrib/bloom, and we surely want to do something about it. The idea
would be to add a page masking routine in IndexAmRoutine and heap to
be able to perform page masking operations directly with that. This
would allow as well one to be able to perform page masking for bloom
or any custom access method, and this will allow this sanity check to
be more generic as well.

Another pin-point is: given a certain page, how do we identify of
which type it is? One possibility would be again to extend the AM
handler with some kind of is_self function with a prototype like that:
bool handler->is_self(Page);
If the page is of the type of the handler, this returns true, and
false otherwise. Still here performance would suck.

At the end, what we want is a clean interface, and more thoughts into it.
--
Michael

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

On Mon, Aug 22, 2016 at 9:25 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Another pin-point is: given a certain page, how do we identify of
which type it is? One possibility would be again to extend the AM
handler with some kind of is_self function with a prototype like that:
bool handler->is_self(Page);
If the page is of the type of the handler, this returns true, and
false otherwise. Still here performance would suck.

At the end, what we want is a clean interface, and more thoughts into it.

I think that it makes sense to filter based on the resource manager
ID, but I can't see how we could reasonably filter based on the AM
name.

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

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

On 22 August 2016 at 13:44, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

Please let me know your thoughts on this.

Do the regression tests pass with this option enabled?

--
Simon Riggs http://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 Mon, Aug 22, 2016 at 9:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Aug 22, 2016 at 9:25 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Another pin-point is: given a certain page, how do we identify of
which type it is? One possibility would be again to extend the AM
handler with some kind of is_self function with a prototype like that:
bool handler->is_self(Page);
If the page is of the type of the handler, this returns true, and
false otherwise. Still here performance would suck.

At the end, what we want is a clean interface, and more thoughts into it.

I think that it makes sense to filter based on the resource manager
ID

+1.

I think the patch currently addresses only a subset of resource
manager id's (mainly Heap and Index resource manager id's). Do we
want to handle the complete resource manager list as defined in
rmgrlist.h?

Another thing that needs some thoughts is the UI of this patch,
currently it is using integer mask which might not be best way, but
again as it is intended to be mainly used for tests, it might be okay.

Do we want to enable some tests in the regression suite by using this option?

--
With Regards,
Amit Kapila.
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

Yes, I've verified the outputs and log contents after running gmake
installcheck and gmake installcheck-world. The status of the test was
marked as pass for all the testcases.

On Mon, Aug 22, 2016 at 9:26 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 22 August 2016 at 13:44, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

Please let me know your thoughts on this.

Do the regression tests pass with this option enabled?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Thanks & Regards,
Kuntal Ghosh

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

On Tue, Aug 23, 2016 at 1:32 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Aug 22, 2016 at 9:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Aug 22, 2016 at 9:25 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Another pin-point is: given a certain page, how do we identify of
which type it is? One possibility would be again to extend the AM
handler with some kind of is_self function with a prototype like that:
bool handler->is_self(Page);
If the page is of the type of the handler, this returns true, and
false otherwise. Still here performance would suck.

At the end, what we want is a clean interface, and more thoughts into it.

I think that it makes sense to filter based on the resource manager
ID

+1.

Yes actually that's better. That's simple enough and removes any need
to looking at pd_special.

I think the patch currently addresses only a subset of resource
manager id's (mainly Heap and Index resource manager id's). Do we
want to handle the complete resource manager list as defined in
rmgrlist.h?

Not all of them generate FPWs. I don't think it matters much.

Another thing that needs some thoughts is the UI of this patch,
currently it is using integer mask which might not be best way, but
again as it is intended to be mainly used for tests, it might be okay.

What we'd want to have is a way to visualize easily differences of
pages. Any other ideas than MASK_MARKER would be welcome of course.

Do we want to enable some tests in the regression suite by using this option?

We could get out a recovery test that sets up a standby/master and
runs the tests of src/test/regress with pg_regress with this parameter
enabled.

+ * bufmask.c
+ *      Routines for buffer masking, used to ensure that buffers used for
+ *      comparison across nodes are in a consistent state.
+ *
+ * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
Copyright notices need to be updated. (It's already been 2 years!!)

Also, what's the use case of allowing only a certain set of rmgrs to
be checked. Wouldn't a simple on/off switch be simpler? As presented,
wal_consistency_mask is also going to be very quite confusing for
users. You should not need to apply some maths to set up this
parameter, a list of rmgr names may be more adapted if this level of
tuning is needed, still it seems to me that we don't need this much.
--
Michael

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

On Tue, Aug 23, 2016 at 10:57 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Also, what's the use case of allowing only a certain set of rmgrs to
be checked. Wouldn't a simple on/off switch be simpler?

I think there should be a way to test WAL for one particular resource
manager. For example, if someone develops a new index or some other
heap storage, only that particular module can be verified. Generating
WAL for all the resource managers together can also serve the purpose,
but it will be slightly difficult to verify it.

As presented,
wal_consistency_mask is also going to be very quite confusing for
users. You should not need to apply some maths to set up this
parameter, a list of rmgr names may be more adapted if this level of
tuning is needed,

Yeah, that can be better.

--
With Regards,
Amit Kapila.
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 22 August 2016 at 16:56, Simon Riggs <simon@2ndquadrant.com> wrote:

On 22 August 2016 at 13:44, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

Please let me know your thoughts on this.

Do the regression tests pass with this option enabled?

Hi,

I'd like to be a reviewer on this. Please can you add this onto the CF
app so we can track the review?

Please supply details of the testing and test coverage.

Thanks

--
Simon Riggs http://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

Hi,

I've added the feature in CP app. Following are the testing details:

1. In master, I've enabled following configurations:

* wal_level = replica
* max_wal_senders = 3
* wal_keep_segments = 4000
* hot_standby = on
* wal_consistency_mask = 511 /* Enable consistency check mask bit*/

2. In slave, I've enabled following configurations:

* standby_mode = on
* wal_consistency_mask = 511 /* Enable consistency check mask bit*/

3. Then, I performed gmake installcheck in master. I didn't get any
warning regarding WAL inconsistency in slave.

I've made following changes to the attached patch:

1. For BRIN pages, I've masked the unused space, PD_PAGE_FULL and
PD_HAS_FREE_LINES flags.
2. For Btree pages, I've masked BTP_HALF_DEAD, BTP_SPLIT_END,
BTP_HAS_GARBAGE and BTP_INCOMPLETE_SPLIT flags.
3. For GIN_DELETED page, I've masked the entire page since the page is
always initialized during recovery.
4. For Speculative Heap tuple insert operation, there was
inconsistency in t_ctid value. So, I've modified the t_ctid value (in
backup page) to current block number and offset number. Need
suggestions!!

What needs to be done:
1. Add support for other Resource Managers.
2. Modify masking techniques for existing Resource Managers (if required).
3. Modify the GUC parameter which will accept a list of rmgr names.
4. Modify the technique for identifying rmgr names for which the
feature should be enabled.
5. Generalize the page type identification technique.

On Wed, Aug 24, 2016 at 2:14 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 22 August 2016 at 16:56, Simon Riggs <simon@2ndquadrant.com> wrote:

On 22 August 2016 at 13:44, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

Please let me know your thoughts on this.

Do the regression tests pass with this option enabled?

Hi,

I'd like to be a reviewer on this. Please can you add this onto the CF
app so we can track the review?

Please supply details of the testing and test coverage.

Thanks

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Thanks & Regards,
Kuntal Ghosh

Kuntal Ghosh wrote:

4. For Speculative Heap tuple insert operation, there was
inconsistency in t_ctid value. So, I've modified the t_ctid value (in
backup page) to current block number and offset number. Need
suggestions!!

In speculative insertions, t_ctid is used to store the speculative
token. I think you should just mask that field out in that case (which
you can recognize because ip_posid is set to magic value 0xfffe).

--
�lvaro Herrera http://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

Thanks a lot.

I just want to mention the situation where I was getting the
speculative token related inconsistency.

ItemPointer in backup page from master:
LOG: ItemPointer BlockNumber: 1 OffsetNumber:65534 Speculative: true
CONTEXT: xlog redo at 0/127F4A48 for Heap/INSERT+INIT: off 1

ItemPointer in current page from slave after redo:
LOG: ItemPointer BlockNumber: 0 OffsetNumber:1 Speculative: false
CONTEXT: xlog redo at 0/127F4A48 for Heap/INSERT+INIT: off 1

As the block numbers are different, I was getting the following warning:
WARNING: Inconsistent page (at byte 8166) found for record
0/127F4A48, rel 1663/16384/16946, forknum 0, blkno 0, Backup Page
Header : (pd_lower: 28 pd_upper: 8152 pd_special: 8192) Current Page
Header: (pd_lower: 28 pd_upper: 8152 pd_special: 8192)
CONTEXT: xlog redo at 0/127F4A48 for Heap/INSERT+INIT: off 1

In heap_xlog_insert, t_ctid is always set to blkno and xlrec->offnum.
I think this is why I was getting the above warning.

On Thu, Aug 25, 2016 at 10:33 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Kuntal Ghosh wrote:

4. For Speculative Heap tuple insert operation, there was
inconsistency in t_ctid value. So, I've modified the t_ctid value (in
backup page) to current block number and offset number. Need
suggestions!!

In speculative insertions, t_ctid is used to store the speculative
token. I think you should just mask that field out in that case (which
you can recognize because ip_posid is set to magic value 0xfffe).

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

--
Thanks & Regards,
Kuntal Ghosh

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

Kuntal Ghosh wrote:

Thanks a lot.

I just want to mention the situation where I was getting the
speculative token related inconsistency.

ItemPointer in backup page from master:
LOG: ItemPointer BlockNumber: 1 OffsetNumber:65534 Speculative: true
CONTEXT: xlog redo at 0/127F4A48 for Heap/INSERT+INIT: off 1

ItemPointer in current page from slave after redo:
LOG: ItemPointer BlockNumber: 0 OffsetNumber:1 Speculative: false
CONTEXT: xlog redo at 0/127F4A48 for Heap/INSERT+INIT: off 1

As the block numbers are different, I was getting the following warning:
WARNING: Inconsistent page (at byte 8166) found for record
0/127F4A48, rel 1663/16384/16946, forknum 0, blkno 0, Backup Page
Header : (pd_lower: 28 pd_upper: 8152 pd_special: 8192) Current Page
Header: (pd_lower: 28 pd_upper: 8152 pd_special: 8192)
CONTEXT: xlog redo at 0/127F4A48 for Heap/INSERT+INIT: off 1

In heap_xlog_insert, t_ctid is always set to blkno and xlrec->offnum.
I think this is why I was getting the above warning.

Umm, really? Then perhaps this *is* a bug. Peter?

--
�lvaro Herrera http://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

Hi Kuntal,

Thanks for the patch.

Current patch has no docs, no tests and no explanatory comments, so
makes review quite hard.

The good news is you might discover a few bugs with it, so its worth
pursuing actively in this CF, though its not near to being
committable.

I think you should add this as part of the default testing for both
check and installcheck. I can't imagine why we'd have it and not use
it during testing.

On 25 August 2016 at 18:41, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

* wal_consistency_mask = 511 /* Enable consistency check mask bit*/

What does this mean? (No docs)

What needs to be done:
1. Add support for other Resource Managers.

We probably need to have a discussion as to why you think this should
be Rmgr dependent?
Code comments would help there.

If it does, then you should probably do this by extending RmgrTable
with an rm_check, so you can call it like this...

RmgrTable[record->xl_rmid].rm_check

I'm interested in how we handle the new generic WAL format for blocks.
Surely if we can handle that then we won't need an Rmgr dependency?
I'm sure you have reasons, they just need to be explained long hand -
don't assume anything.

5. Generalize the page type identification technique.

Why not do this first?

There are some coding guideline stuff to check as well.

Thanks

--
Simon Riggs http://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 Fri, Aug 26, 2016 at 9:26 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

I think you should add this as part of the default testing for both
check and installcheck. I can't imagine why we'd have it and not use
it during testing.

The actual consistency checks are done during redo (replay), so not
sure whats in you mind for enabling it with check or installcheck. I
think we can run few recovery/replay tests with this framework. Also
running the tests under this framework could be time-consuming as it
logs the entire page for each WAL record we write and then during
replay reads the same.

--
With Regards,
Amit Kapila.
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 27 August 2016 at 07:36, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Aug 26, 2016 at 9:26 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

I think you should add this as part of the default testing for both
check and installcheck. I can't imagine why we'd have it and not use
it during testing.

The actual consistency checks are done during redo (replay), so not
sure whats in you mind for enabling it with check or installcheck. I
think we can run few recovery/replay tests with this framework. Also
running the tests under this framework could be time-consuming as it
logs the entire page for each WAL record we write and then during
replay reads the same.

I'd like to see an automated test added so we can be certain we don't
add things that break recovery. Don't mind much where or how.

The main use is to maintain that certainty while in production.

--
Simon Riggs http://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

Hello Simon,

I'm really sorry for the inconveniences. Next time, I'll attach the
patch with proper documentation, test and comments.

I think you should add this as part of the default testing for both
check and installcheck. I can't imagine why we'd have it and not use
it during testing.

Since, this is redo(replay) feature, we can surely add this in
installcheck. But, as Amit mentioned, it could be time-consuming.

* wal_consistency_mask = 511 /* Enable consistency check mask bit*/

What does this mean? (No docs)

I was using this parameter as a masking integer to indicate the
operations(rmgr list) for which we need this feature to be enabled.
Since, this could be confusing, I've changed it accordingly so that it
accepts a list of rmgrIDs. (suggested by Michael, Amit and Robert)

1. Add support for other Resource Managers.

We probably need to have a discussion as to why you think this should
be Rmgr dependent?
Code comments would help there.

If it does, then you should probably do this by extending RmgrTable
with an rm_check, so you can call it like this...

RmgrTable[record->xl_rmid].rm_check

+1.
I'm modifying it accordingly. I'm calling this function after
RmgrTable[record->xl_rmid].rm_redo.

5. Generalize the page type identification technique.

Why not do this first?

At present, I'm using special page size and page ID to identify page
type. But, I've noticed some cases where the entire page is
initialized to zero (Ex: hash_xlog_squeeze_page). RmgrID and info bit
can help us to identify those pages.

--
Thanks & Regards,
Kuntal Ghosh
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 Sat, Aug 27, 2016 at 6:16 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 27 August 2016 at 07:36, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Aug 26, 2016 at 9:26 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

I think you should add this as part of the default testing for both
check and installcheck. I can't imagine why we'd have it and not use
it during testing.

The actual consistency checks are done during redo (replay), so not
sure whats in you mind for enabling it with check or installcheck. I
think we can run few recovery/replay tests with this framework. Also
running the tests under this framework could be time-consuming as it
logs the entire page for each WAL record we write and then during
replay reads the same.

I'd like to see an automated test added so we can be certain we don't
add things that break recovery. Don't mind much where or how.

The main use is to maintain that certainty while in production.

For developers, having extra checks with the new routines in WAL_DEBUG
could also be useful for a code path producing WAL. Let's not forget
that as well.
--
Michael

--
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, Aug 26, 2016 at 7:24 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

As the block numbers are different, I was getting the following warning:
WARNING: Inconsistent page (at byte 8166) found for record
0/127F4A48, rel 1663/16384/16946, forknum 0, blkno 0, Backup Page
Header : (pd_lower: 28 pd_upper: 8152 pd_special: 8192) Current Page
Header: (pd_lower: 28 pd_upper: 8152 pd_special: 8192)
CONTEXT: xlog redo at 0/127F4A48 for Heap/INSERT+INIT: off 1

In heap_xlog_insert, t_ctid is always set to blkno and xlrec->offnum.
I think this is why I was getting the above warning.

Umm, really? Then perhaps this *is* a bug. Peter?

It's a matter of perspective, but probably not. The facts are:

* heap_insert() treats speculative insertions differently. In
particular, it does not set ctid in the caller-passed heap tuple
itself, leaving the ctid field to contain a speculative token value --
a per-backend monotonically increasing identifier. This identifier
represents some particular speculative insertion attempt within a
backend.

* On the redo side, heap_xlog_insert() was only changed mechanically
when upsert went in. So, it doesn't actually care about the stuff that
heap_insert() was made to care about to support speculative insertion.

* Furthermore, heap_insert() does *not* WAL log ctid under any
circumstances (that didn't change, either). Traditionally, the ctid
field was completely redundant anyway (since, of course, we're only
dealing with newly inserted tuples in heap_insert()). With speculative
insertions, there is a token within ctid, whose value represents
actual information that cannot be reconstructed after the fact (the
speculative token value). Even still, that isn't WAL-logged (see
comments above xl_heap_header struct). That's okay, because the
speculative insertion token value is only needed due to obscure issues
with "unprincipled deadlocks". The speculative token value itself is
only of interest to other, conflicting inserters, and only for the
instant in time immediately following physical insertion. The token
doesn't matter in the slightest to crash recovery, nor to Hot Standby
replicas.

While this design had some really nice properties (ask me if you are
unclear on this), it does break tools like the proposed WAL-checker
tool. I would compare this speculative token situation to the
situation with hint bits (when checksums are disabled, and
wal_log_hints = off).

I actually have a lot of sympathy for the idea that, in general, cases
like this should be avoided. But, would it really be worth it to
create a useless special case for speculative insertion (to WAL-log
the virtually useless speculative insertion token value)? I'm certain
that the answer must be "no": This tool ought to deal with speculative
insertion as a special case, and not vice-versa.

--
Peter Geoghegan

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

On Thu, Aug 25, 2016 at 9:41 AM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:

2. For Btree pages, I've masked BTP_HALF_DEAD, BTP_SPLIT_END,
BTP_HAS_GARBAGE and BTP_INCOMPLETE_SPLIT flags.

Why? I think that you should only perform this kind of masking where
it's clearly strictly necessary.

It is true that nbtree can allow cases where LP_DEAD is set with only
a share lock (by read-only queries), so I can see why BTP_HAS_GARBAGE
might need to be excluded; this is comparable to the heapam's use of
hint bits. However, it is unclear why you need to mask the remaining
btpo_flags that you list, because the other flags have clear-cut roles
in various atomic operations that we WAL-log.

--
Peter Geoghegan

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