Thank you Robert. Sure, I will add the updated patch on the next CommitFest with all the suggested changes.
On Wed, Apr 6, 2016 at 9:06 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Wed, Apr 6, 2016 at 3:32 AM, Asif Naeem <anaeem...@gmail.com> wrote: > >> Oh, I see. I think it's probably not a good idea to skip truncating > >> those maps, but perhaps the option could be defined as making no new > >> entries, rather than ignoring them altogether, so that they never > >> grow. It seems that both of those are defined in such a way that if > >> page Y follows page X in the heap, the entries for page Y in the > >> relation fork will follow the one for page X. So truncating them > >> should be OK; it's just growing them that we need to avoid. > > > > Thank you Robert. PFA basic patch, it introduces EMERGENCY option to > VACUUM > > that forces to avoid extend any entries in the VM or FSM. It seems > working > > fine in simple test scenarios e.g. > > > >> postgres=# create table test1 as (select generate_series(1,100000)); > >> SELECT 100000 > >> postgres=# vacuum EMERGENCY test1; > >> VACUUM > >> postgres=# select pg_relation_filepath('test1'); > >> pg_relation_filepath > >> ---------------------- > >> base/13250/16384 > >> (1 row) > >> [asif@centos66 inst_96]$ find . | grep base/13250/16384 > >> ./data/base/13250/16384 > >> postgres=# vacuum test1; > >> VACUUM > >> [asif@centos66 inst_96]$ find . | grep base/13250/16384 > >> ./data/base/13250/16384 > >> ./data/base/13250/16384_fsm > >> ./data/base/13250/16384_vm > > > > > > Please do let me know if I missed something or more information is > required. > > Thanks. > > This is too late for 9.6 at this point and certainly requires > discussion anyway, so please add it to the next CommitFest. But in > the meantime, here are a few quick comments: > > You should only support EMERGENCY using the new-style, parenthesized > options syntax. That way, you won't need to make EMERGENCY a keyword. > We don't want to do that, and we especially don't want it to be > partially reserved, as you have done here. > > Passing the EMERGENCY flag around in a global variable is probably not > a good idea; use parameter lists. That's what they are for. Also, > calling the variable that decides whether EMERGENCY was set > Extend_VM_FSM is confusing. > > I see why you changed the calling convention for visibilitymap_pin() > and RecordPageWithFreeSpace(), but that's awfully invasive. I wonder > if there's a better way to do that, like maybe having vacuumlazy.c ask > the VM and FSM for their length in pages and then not trying to use > those functions for block numbers that are too large. > > Don't forget to update the documentation. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >