pg_filedump 9.3: checksums (and a few other fixes)

Jeff Davis wrote:

--- 1000,1015 ----
strcat (flagString, "HASEXTERNAL|");
if (infoMask & HEAP_HASOID)
strcat (flagString, "HASOID|");
+ 	  if (infoMask & HEAP_XMAX_KEYSHR_LOCK)
+ 	    strcat (flagString, "XMAX_KEYSHR_LOCK|");
if (infoMask & HEAP_COMBOCID)
strcat (flagString, "COMBOCID|");
if (infoMask & HEAP_XMAX_EXCL_LOCK)
strcat (flagString, "XMAX_EXCL_LOCK|");
! 	  if (infoMask & HEAP_XMAX_SHR_LOCK)
! 	    strcat (flagString, "XMAX_SHR_LOCK|");
! 	  if (infoMask & HEAP_XMAX_LOCK_ONLY)
! 	    strcat (flagString, "XMAX_LOCK_ONLY|");
if (infoMask & HEAP_XMIN_COMMITTED)
strcat (flagString, "XMIN_COMMITTED|");
if (infoMask & HEAP_XMIN_INVALID)

Hm, note that XMAX_SHR_LOCK is two bits, so when that flag is present
you will get the three lock modes displayed with the above code, which is
probably going to be misleading. htup_details.h does this:

/*
* Use these to test whether a particular lock is applied to a tuple
*/
#define HEAP_XMAX_IS_SHR_LOCKED(infomask) \
(((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_SHR_LOCK)
#define HEAP_XMAX_IS_EXCL_LOCKED(infomask) \
(((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_EXCL_LOCK)
#define HEAP_XMAX_IS_KEYSHR_LOCKED(infomask) \
(((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_KEYSHR_LOCK)

Presumably it'd be better to do something similar.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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, 2013-06-10 at 01:28 -0400, Alvaro Herrera wrote:

Hm, note that XMAX_SHR_LOCK is two bits, so when that flag is present
you will get the three lock modes displayed with the above code, which is
probably going to be misleading. htup_details.h does this:

/*
* Use these to test whether a particular lock is applied to a tuple
*/
#define HEAP_XMAX_IS_SHR_LOCKED(infomask) \
(((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_SHR_LOCK)
#define HEAP_XMAX_IS_EXCL_LOCKED(infomask) \
(((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_EXCL_LOCK)
#define HEAP_XMAX_IS_KEYSHR_LOCKED(infomask) \
(((infomask) & HEAP_LOCK_MASK) == HEAP_XMAX_KEYSHR_LOCK)

Presumably it'd be better to do something similar.

I was hesitant to do too much interpretation of the bits. Do you think
it would be better to just remove the test for XMAX_SHR_LOCK?

Regards,
Jeff Davis

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

Jeff Davis wrote:

I was hesitant to do too much interpretation of the bits. Do you think
it would be better to just remove the test for XMAX_SHR_LOCK?

I don't know, but then I'm biased because I know what that specific bit
combination means. I guess someone that doesn't know is going to be
surprised by seeing both lock strength bits together .. but maybe
they're just going to have a look at htup_details.h and instantly
understand what's going on. Not sure how likely that is.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Jeff Davis wrote:

I was hesitant to do too much interpretation of the bits. Do you think
it would be better to just remove the test for XMAX_SHR_LOCK?

I don't know, but then I'm biased because I know what that specific bit
combination means. I guess someone that doesn't know is going to be
surprised by seeing both lock strength bits together .. but maybe
they're just going to have a look at htup_details.h and instantly
understand what's going on. Not sure how likely that is.

I think it's all right because there are only 4 combinations of the two
bits and all 4 will be printed sensibly if we do it this way. It would
be bad if pg_filedump could print invalid flag combinations in a
misleading way --- but I don't see such a risk here. So we might as
well go for readability.

The thing I'm not too happy about is having to copy the checksum code
into pg_filedump. We just got rid of the need to do that for the CRC
code, and here it is coming back again. Can't we rearrange the core
checksum code similarly to what we did for the CRC stuff recently,
so that you only need access to a .h file for it?

regards, tom lane

--
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, 2013-06-10 at 11:38 -0400, Tom Lane wrote:

The thing I'm not too happy about is having to copy the checksum code
into pg_filedump. We just got rid of the need to do that for the CRC
code, and here it is coming back again. Can't we rearrange the core
checksum code similarly to what we did for the CRC stuff recently,
so that you only need access to a .h file for it?

The CRC implementation is entirely in header files. Do you think we need
to go that far, or is it fine to just put it in libpgport and link that
to pg_filedump?

Regards,
Jeff Davis

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

Jeff Davis wrote:

On Mon, 2013-06-10 at 11:38 -0400, Tom Lane wrote:

The thing I'm not too happy about is having to copy the checksum code
into pg_filedump. We just got rid of the need to do that for the CRC
code, and here it is coming back again. Can't we rearrange the core
checksum code similarly to what we did for the CRC stuff recently,
so that you only need access to a .h file for it?

The CRC implementation is entirely in header files. Do you think we need
to go that far, or is it fine to just put it in libpgport and link that
to pg_filedump?

If a lib is okay, use libpgcommon please, not libpgport. But I think a
.h would be better, because there'd be no need to have a built source
tree to build pg_filedump, only the headers installed.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Jeff Davis wrote:

The CRC implementation is entirely in header files. Do you think we need
to go that far, or is it fine to just put it in libpgport and link that
to pg_filedump?

If a lib is okay, use libpgcommon please, not libpgport. But I think a
.h would be better, because there'd be no need to have a built source
tree to build pg_filedump, only the headers installed.

Neither lib would provide a useful solution so far as Red Hat's
packaging is concerned, because those libs are not exposed to other
packages (and never will be, unless we start supplying them as .so's)

regards, tom lane

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

Jeff Davis <pgsql@j-davis.com> writes:

The patch is a bit ugly: I had to copy some code, and copy the entire
checksum.c file (minus some Asserts, which don't work in an external
program). Suggestions welcome.

What I propose we do about this is reduce backend/storage/page/checksum.c
to something like

#include "postgres.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"

moving all the code currently in the file into a new .h file. Then,
any external programs such as pg_filedump can use the checksum code
by including checksum_impl.h. This is essentially the same thing we
did with the CRC support functionality some time ago.

Also, we have the cut-point between checksum.c and bufpage.c at the
wrong place. IMO we should move PageCalcChecksum16 in toto into
checksum.c (or really now into checksum_impl.h), because that and not
just checksum_block() is the functionality that is wanted.

regards, tom lane

--
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, 2013-06-13 at 20:09 -0400, Tom Lane wrote:

What I propose we do about this is reduce backend/storage/page/checksum.c
to something like

#include "postgres.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"

moving all the code currently in the file into a new .h file. Then,
any external programs such as pg_filedump can use the checksum code
by including checksum_impl.h. This is essentially the same thing we
did with the CRC support functionality some time ago.

Thank you for taking care of that. After seeing that it needed to be in
a header file, I was going to try doing it all as macros.

I have a question about the commit though: shouldn't both functions be
static if they are in a .h file? Otherwise, it could lead to naming
conflicts. I suppose it's wrong to include the implementation file
twice, but it still might be confusing if someone tries. Two ideas that
come to mind are:
* make both static and then have a trivial wrapper in checksum.c
* export one or both functions, but use #ifndef CHECKSUM_IMPL_H to
prevent redefinition

Also, we have the cut-point between checksum.c and bufpage.c at the
wrong place. IMO we should move PageCalcChecksum16 in toto into
checksum.c (or really now into checksum_impl.h), because that and not
just checksum_block() is the functionality that is wanted.

Agreed.

Regards,
Jeff Davis

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

Jeff Davis <pgsql@j-davis.com> writes:

I have a question about the commit though: shouldn't both functions be
static if they are in a .h file? Otherwise, it could lead to naming
conflicts. I suppose it's wrong to include the implementation file
twice, but it still might be confusing if someone tries. Two ideas that
come to mind are:
* make both static and then have a trivial wrapper in checksum.c
* export one or both functions, but use #ifndef CHECKSUM_IMPL_H to
prevent redefinition

Ah, you are right, I forgot the #ifndef CHECKSUM_IMPL_H dance. Will fix
in a bit.

regards, tom lane

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

On 2013-06-14 11:59:04 -0400, Tom Lane wrote:

Jeff Davis <pgsql@j-davis.com> writes:

I have a question about the commit though: shouldn't both functions be
static if they are in a .h file? Otherwise, it could lead to naming
conflicts. I suppose it's wrong to include the implementation file
twice, but it still might be confusing if someone tries. Two ideas that
come to mind are:
* make both static and then have a trivial wrapper in checksum.c
* export one or both functions, but use #ifndef CHECKSUM_IMPL_H to
prevent redefinition

Ah, you are right, I forgot the #ifndef CHECKSUM_IMPL_H dance. Will fix
in a bit.

That won't help against errors if it's included in two different
files/translation units though. I don't really see a valid case where it
could be validly be included multiple times in one TU?
If anything we should #error in that case, but I am not sure it's worth
bothering.
E.g. in rmgrlist.h we have the following comment:
/* there is deliberately not an #ifndef RMGRLIST_H here */
and I think the reasoning behind that comment applies here as well.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

Andres Freund <andres@2ndquadrant.com> writes:

On 2013-06-14 11:59:04 -0400, Tom Lane wrote:

Ah, you are right, I forgot the #ifndef CHECKSUM_IMPL_H dance. Will fix
in a bit.

That won't help against errors if it's included in two different
files/translation units though.

Good point, but there's not any real reason to do that --- only
checksum.h should ever be #include'd in more than one file. Any program
using this stuff is expected to #include checksum_impl.h in exactly one
place. So maybe it's fine as-is.

E.g. in rmgrlist.h we have the following comment:
/* there is deliberately not an #ifndef RMGRLIST_H here */
and I think the reasoning behind that comment applies here as well.

Well, that's a different case: there, and also in kwlist.h, there's an
idea that it could actually be useful to #include the file more than
once, redefining the PG_RMGR() macro each time. There's no such use
case that I can see for checksum_impl.h.

regards, tom lane

--
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, 2013-06-13 at 20:09 -0400, Tom Lane wrote:

What I propose we do about this is reduce backend/storage/page/checksum.c
to something like

#include "postgres.h"
#include "storage/checksum.h"
#include "storage/checksum_impl.h"

Attached a new diff for pg_filedump that makes use of the above change.

I'm not sure what the resolution of Alvaro's concern was, so I left the
flag reporting the same as the previous patch.

Regards,
Jeff Davis

On Tue, Jun 18, 2013 at 12:42 PM, Jeff Davis <pgsql@j-davis.com> wrote:

Attached a new diff for pg_filedump that makes use of the above change.

I'm not sure what the resolution of Alvaro's concern was, so I left the
flag reporting the same as the previous patch.

This patch is in the current CommitFest, does it still need to be
reviewed? If so, I notice that the version in pgfoundry's CVS is
rather different than the version the patch seems to have been built
against (presumably the pg_filedump-9.2.0.tar.gz release), and
conflicts in several places with cvs tip.

Also, would anyone be willing to convert this repository to git and
post it on github or similar? pgfoundry is becoming increasingly
difficult to use, for instance the 'Browse CVS Repository' link for
pg_filedump and other projects is broken[1]http://pgfoundry.org/scm/browser.php?group_id=1000541 and apparently has been
for months[2]http://pgfoundry.org/forum/forum.php?thread_id=15554&amp;forum_id=44&amp;group_id=1000013, not to mention the general crumminess of using CVS [3]Since the pgfoundry cvs server apparently doesn't support the 'ls' command, you might need to know this to know which module name to check out: cvs -d :pserver:anonymous@cvs.pgfoundry.org:/cvsroot/pgfiledump checkout pg_filedump.

Josh

[1]: http://pgfoundry.org/scm/browser.php?group_id=1000541
[2]: http://pgfoundry.org/forum/forum.php?thread_id=15554&amp;forum_id=44&amp;group_id=1000013
[3]: Since the pgfoundry cvs server apparently doesn't support the 'ls' command, you might need to know this to know which module name to check out: cvs -d :pserver:anonymous@cvs.pgfoundry.org:/cvsroot/pgfiledump checkout pg_filedump
command, you might need to know this to know which module name to
check out: cvs -d
:pserver:anonymous@cvs.pgfoundry.org:/cvsroot/pgfiledump checkout
pg_filedump

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

Josh Kupershmidt <schmiddy@gmail.com> writes:

This patch is in the current CommitFest, does it still need to be
reviewed? If so, I notice that the version in pgfoundry's CVS is
rather different than the version the patch seems to have been built
against (presumably the pg_filedump-9.2.0.tar.gz release), and
conflicts in several places with cvs tip.

Yeah, I pushed some basic 9.3 compatibility fixes into pg_filedump CVS
a few weeks back. If someone could rebase the patch against that,
I'd appreciate it.

Also, would anyone be willing to convert this repository to git and
post it on github or similar?

I know it's time to get off of pgfoundry, but doing so for pg_filedump
is way down the priority list ...

regards, tom lane

--
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, 2013-06-24 at 20:34 -0400, Josh Kupershmidt wrote:

This patch is in the current CommitFest, does it still need to be
reviewed? If so, I notice that the version in pgfoundry's CVS is
rather different than the version the patch seems to have been built
against (presumably the pg_filedump-9.2.0.tar.gz release), and
conflicts in several places with cvs tip.

Oh, thank you. After browsing the CVS repo failed, I just made the diff
against 9.2.0. I'll rebase it.

Regards,
Jeff Davis

--
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, 2013-06-24 at 20:34 -0400, Josh Kupershmidt wrote:

This patch is in the current CommitFest, does it still need to be
reviewed? If so, I notice that the version in pgfoundry's CVS is
rather different than the version the patch seems to have been built
against (presumably the pg_filedump-9.2.0.tar.gz release), and
conflicts in several places with cvs tip.

Rebased against CVS tip; attached.

Also, would anyone be willing to convert this repository to git and
post it on github or similar? pgfoundry is becoming increasingly
difficult to use, for instance the 'Browse CVS Repository' link for
pg_filedump and other projects is broken[1] and apparently has been
for months[2], not to mention the general crumminess of using CVS [3].

Eventually, it would be nice to have a more full-featured offline
checker utility. Do we want to try to turn this utility into that, or
make a new one?

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

On Mon, 2013-06-24 at 20:34 -0400, Josh Kupershmidt wrote:

This patch is in the current CommitFest, does it still need to be
reviewed? If so, I notice that the version in pgfoundry's CVS is
rather different than the version the patch seems to have been built
against (presumably the pg_filedump-9.2.0.tar.gz release), and
conflicts in several places with cvs tip.

Rebased against CVS tip; attached.

Thanks. I'm feeling pretty overworked these days but will try to push
this into pgfoundry in a timely fashion.

Eventually, it would be nice to have a more full-featured offline
checker utility. Do we want to try to turn this utility into that, or
make a new one?

TBH, I've always been annoyed that pg_filedump is GPL and so there's no
way for us to just ship it in contrib. (That stems from Red Hat
corporate policy of a dozen years ago, but the conflict is real anyway.)
If somebody is sufficiently excited about this topic to do something
that's largely new anyway, I'd be in favor of starting from scratch so
it could be put under the usual Postgres license.

regards, tom lane

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

On Wed, Jun 26, 2013 at 8:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

TBH, I've always been annoyed that pg_filedump is GPL and so there's no
way for us to just ship it in contrib. (That stems from Red Hat
corporate policy of a dozen years ago, but the conflict is real anyway.)
If somebody is sufficiently excited about this topic to do something
that's largely new anyway, I'd be in favor of starting from scratch so
it could be put under the usual Postgres license.

Heroku are interested in online verification of basebackups (i.e.
using checksums to verify the integrity of heap files as they are
backed up, with a view to relying less and less on logical backups). I
am very glad that you made the page checksums stuff available to
external utilities in commit f04216341dd1cc235e975f93ac806d9d3729a344.

In the last couple of days, I haven't been able to figure out a way to
solve the problem of torn pages in a way that isn't a complete kludge
(with a hopefully-acceptable risk of false positives), so I've been
operating under the assumption that anything I produce here won't be
up to the standards of contrib. I had intended to release whatever
results as an open source project anyway. However, if we can figure
out a way to solve the torn pages problem, or at least produce
something acceptable, I think I'd certainly be able to find the time
to work on a contrib module that is mainly concerned with verifying
basebackups, but also offers some pg_filedump-like functionality.
That's something largely new.

--
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