On Thu, Mar 10, 2016 at 1:41 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Fri, Mar 11, 2016 at 1:03 AM, Robert Haas <robertmh...@gmail.com> wrote: >> This 001 patch looks so little like what I was expecting that I >> decided to start over from scratch. The new version I wrote is >> attached here. I don't understand why your version tinkers with the >> logic for setting the all-frozen bit; I thought that what I already >> committed dealt with that already, and in any case, your version >> doesn't even compile against latest sources. Your version also leaves >> the scan_all terminology intact even though it's not accurate any >> more, and I am not very convinced that the updates to the >> page-skipping logic are actually correct. Please have a look over >> this version and see what you think. > > Thank you for your advise. > Sorry, optimising logic of previous patch was old by mistake. > Attached latest patch incorporated your suggestions with a little revising.
OK, I'll have a look. Thanks. >> I think that's kind of pointless. We need to test that this >> conversion code works, but once it does, I don't think we should make >> everybody pay the overhead of retesting that. Anyway, the test code >> could have bugs, too. >> >> Here's an updated version of your patch with that code removed and >> some cosmetic cleanups like fixing typos and stuff like that. I think >> this is mostly ready to commit, but I noticed one problem: your >> conversion code always produces two output pages for each input page >> even if one of them would be empty. In particular, if you have a >> large number of small relations and run pg_upgrade, all of their >> visibility maps will go from 8kB to 16kB. That isn't the end of the >> world, maybe, but I think you should see if you can't fix it >> somehow.... > > Thank you for updating patch. > To deal with this problem, I've changed it so that pg_upgrade checks > file size before conversion. > And if fork file does not exist or size is 0 (empty), ignore. > Attached latest patch. I think what I really want is some logic so that if we have a 1-page visibility map in the old cluster and the second half of that page is all zeroes, we only create a 1-page visibility map in the new cluster rather than a 2-page visibility map. Or more generally, if the old VM is N pages, but the last half of the last page is empty, then let the output VM be 2*N-1 pages instead of 2*N pages. -- 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
