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:
>
Sure. Sorry for delay caused.
> 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.
>
Sure. I have removed EMERGENCY keyword, it made code lot small now i.e.
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index b9aeb31..89c4ee0 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -9298,6 +9298,20 @@ vacuum_option_elem:
> | VERBOSE { $$ =
> VACOPT_VERBOSE; }
> | FREEZE { $$ =
> VACOPT_FREEZE; }
> | FULL { $$ =
> VACOPT_FULL; }
> + | IDENT
> + {
> + /*
> + * We handle identifiers that
> aren't parser keywords with
> + * the following special-case
> codes.
> + */
> + if (strcmp($1, "emergency") == 0)
> + $$ = VACOPT_EMERGENCY;
> + else
> + ereport(ERROR,
> +
> (errcode(ERRCODE_SYNTAX_ERROR),
> +
> errmsg("unrecognized vacuum option \"%s\"", $1),
> +
> parser_errposition(@1)));
> + }
> ;
>
> AnalyzeStmt:
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.
>
Sure. I adopted this approach by find other similar cases in the same
source code file i.e.
src/backend/commands/vacuumlazy.c
> /* A few variables that don't seem worth passing around as parameters */
> static int elevel = -1;
> static TransactionId OldestXmin;
> static TransactionId FreezeLimit;
> static MultiXactId MultiXactCutoff;
> static BufferAccessStrategy vac_strategy;
Should I modify code to use parameter lists for these variables too ?
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
>
