Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-21 Thread Jeff Davis
On Wed, 2013-07-17 at 13:43 -0400, Alvaro Herrera wrote: Tom Lane escribió: My feeling about this code is that the reason we print the infomask in hex is so you can see exactly which bits are set if you care, and that the rest of the line ought to be designed to interpret the bits in as

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-17 Thread Alvaro Herrera
Tom Lane escribió: Alvaro Herrera alvhe...@2ndquadrant.com writes: Well, Tom opined in http://www.postgresql.org/message-id/23249.1370878...@sss.pgh.pa.us that the current patch is okay. I have a mild opinion that it should instead print only SHR_LOCK when both bits are set, and one of

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-17 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes: The one I was talking about is the second case, which prints KEYSHR_LOCK|EXCL_LOCK to mean that there's a FOR SHARE lock. I have no problem reading it this way, but I fear that someone unfamiliar with these bits might be confused. On the other

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-17 Thread Alvaro Herrera
Tom Lane escribió: My feeling about this code is that the reason we print the infomask in hex is so you can see exactly which bits are set if you care, and that the rest of the line ought to be designed to interpret the bits in as reader-friendly a way as possible. So I don't buy the notion

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-16 Thread Josh Berkus
On 07/08/2013 04:59 PM, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Well, Tom opined in http://www.postgresql.org/message-id/23249.1370878...@sss.pgh.pa.us that the current patch is okay. I have a mild opinion that it should instead print only SHR_LOCK when both bits are

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-16 Thread Tom Lane
Josh Berkus j...@agliodbs.com writes: On 07/08/2013 04:59 PM, Tom Lane wrote: FWIW, I think that's exactly what I did in the preliminary 9.3 patch that I committed to pg_filedump a few weeks ago. Could you take a look at what's there now and see if that's what you meant? So, is this getting

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-08 Thread Jeff Davis
On Fri, 2013-07-05 at 22:43 -0700, Jeff Davis wrote: On Sat, 2013-07-06 at 10:30 +0900, Satoshi Nagayasu wrote: Hi, It looks fine, but I have one question here. When I run pg_filedump with -k against a database cluster which does not support checksums, pg_filedump produced checksum

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-08 Thread Peter Geoghegan
On Mon, Jul 8, 2013 at 10:28 AM, Jeff Davis pg...@j-davis.com wrote: I see this patch is still waiting on author in the CF. Is there something else needed from me, or should we move this to ready for committer? Well, obviously someone still needs to think through the handling of the infoMask

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-08 Thread Alvaro Herrera
Peter Geoghegan escribió: On Mon, Jul 8, 2013 at 10:28 AM, Jeff Davis pg...@j-davis.com wrote: I see this patch is still waiting on author in the CF. Is there something else needed from me, or should we move this to ready for committer? Well, obviously someone still needs to think

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-08 Thread Peter Geoghegan
On Mon, Jul 8, 2013 at 11:52 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Well, Tom opined in http://www.postgresql.org/message-id/23249.1370878...@sss.pgh.pa.us that the current patch is okay. I have a mild opinion that it should instead print only SHR_LOCK when both bits are set, and

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-08 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes: Well, Tom opined in http://www.postgresql.org/message-id/23249.1370878...@sss.pgh.pa.us that the current patch is okay. I have a mild opinion that it should instead print only SHR_LOCK when both bits are set, and one of the others when only one

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-05 Thread Satoshi Nagayasu
Hi, I have reviewed this patch as a CF reviewer. (2013/06/27 4:07), Jeff Davis wrote: 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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-07-05 Thread Jeff Davis
On Sat, 2013-07-06 at 10:30 +0900, Satoshi Nagayasu wrote: Hi, It looks fine, but I have one question here. When I run pg_filedump with -k against a database cluster which does not support checksums, pg_filedump produced checksum error as following. Is this expected or acceptable? Thank

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-28 Thread Jeff Davis
On Thu, 2013-06-27 at 15:59 +0200, Andres Freund wrote: Maybe the trick is to add a recovery.conf option to make postgres replay to the first restartpoint and then shutdown. At that point you can be sure there aren't any torn pages anymore (bugs aside). In fact that sounds like a rather useful

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-27 Thread Andres Freund
On 2013-06-26 21:18:49 -0700, Peter Geoghegan wrote: On Wed, Jun 26, 2013 at 8:27 PM, Tom Lane t...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-27 Thread Peter Geoghegan
On Wed, Jun 26, 2013 at 11:27 PM, Andres Freund and...@2ndquadrant.com wrote: Why not do this from a function/background worker in the backend where you can go via the buffer manager to avoid torn pages et al. If you use a buffer strategy the cache poisoning et al should be controlleable. I

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-27 Thread Andres Freund
On 2013-06-26 23:42:55 -0700, Peter Geoghegan wrote: On Wed, Jun 26, 2013 at 11:27 PM, Andres Freund and...@2ndquadrant.com wrote: Why not do this from a function/background worker in the backend where you can go via the buffer manager to avoid torn pages et al. If you use a buffer

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-27 Thread Peter Geoghegan
On Tue, Jun 18, 2013 at 9:42 AM, Jeff Davis pg...@j-davis.com wrote: I'm not sure what the resolution of Alvaro's concern was, so I left the flag reporting the same as the previous patch. Alvaro's concern was that the new flags added (those added by the foreign key locks patch) do something

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-27 Thread Peter Geoghegan
On Thu, Jun 27, 2013 at 12:07 AM, Peter Geoghegan p...@heroku.com wrote: I'm not sure what the resolution of Alvaro's concern was, so I left the flag reporting the same as the previous patch. Alvaro's concern was that the new flags added (those added by the foreign key locks patch) do

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-27 Thread Andres Freund
On 2013-06-27 09:51:07 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-06-26 21:18:49 -0700, Peter Geoghegan wrote: Heroku are interested in online verification of basebackups (i.e. using checksums to verify the integrity of heap files as they are backed up,

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-27 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-06-26 21:18:49 -0700, Peter Geoghegan wrote: 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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-26 Thread Jeff Davis
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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-26 Thread Tom Lane
Jeff Davis pg...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-26 Thread Peter Geoghegan
On Wed, Jun 26, 2013 at 8:27 PM, Tom Lane t...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-25 Thread Jeff Davis
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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-24 Thread Josh Kupershmidt
On Tue, Jun 18, 2013 at 12:42 PM, Jeff Davis pg...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-24 Thread Tom Lane
Josh Kupershmidt schmi...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-18 Thread Jeff Davis
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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-14 Thread Jeff Davis
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.

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-14 Thread Tom Lane
Jeff Davis pg...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-14 Thread Andres Freund
On 2013-06-14 11:59:04 -0400, Tom Lane wrote: Jeff Davis pg...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-14 Thread Tom Lane
Andres Freund and...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-13 Thread Tom Lane
Jeff Davis pg...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-10 Thread Jeff Davis
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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-10 Thread Alvaro Herrera
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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-10 Thread Tom Lane
Alvaro Herrera alvhe...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-10 Thread Jeff Davis
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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-10 Thread Alvaro Herrera
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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-10 Thread Tom Lane
Alvaro Herrera alvhe...@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

Re: [HACKERS] pg_filedump 9.3: checksums (and a few other fixes)

2013-06-09 Thread Alvaro Herrera
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)