Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
Alvaro Herrera wrote: Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Hmm, I didn't recheck after Greg's patch, but in mine, it doesn't, because the location is saved as reset location and restored when the variable is reset. It worked fine in all cases I tested. Hmm. Actually, why is there a need to save and restore at all? There can certainly never be more than one recorded config-file location per variable. What about saying that each variable has one and only one filename/linenumber, but whether these are relevant to the current value is determined by whether the current value's source is S_FILE? Hmm, this does make the patch a lot simpler. Attached. (Magnus was visionary enough to put the correct test in the pg_settings definition.) :-) Yeah, it does look at lot simpler. And it certainly takes away the pieces of code of mine that I was entirely unable to make working :-) I also dropped the change to set_config_option, and added a new routine to set the source file/line, as you suggested elsewhere. The only problem is that we now have two bsearch calls for each option set in a config file ... I don't want to change set_config_option just to be able to return the struct config_generic for this routine's sake ... Better ideas? Is this OK as is? Well, it's not like it's a performance critical path, is it? I think we should be ok. I noticed some weird things when the config files contain errors, but I think it's outside this patch's scope. (I dropped the default stuff for now, as it doesn't seem that a consensus has been reached on that topic.) This is one of the reasons I suggested keeping that one as a separate patch in the first place. The other main reason being that once it gets applied, you really want it to be two different revisions, to clearly keep them apart ;-) I still think we should eventually get both in there, but let's treat them as separate entities. //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
On Tue, 9 Sep 2008, Magnus Hagander wrote: (I dropped the default stuff for now, as it doesn't seem that a consensus has been reached on that topic.) This is one of the reasons I suggested keeping that one as a separate patch in the first place. The other main reason being that once it gets applied, you really want it to be two different revisions, to clearly keep them apart This means some committer is going to have to make a second pass over the same section of code and do testing there more than once, that's a waste of time I was trying to avoid. Also, any standalone patch I submit right now won't apply cleanly if the source file/line patch is committed. If nobody cares about doing that work twice, I'll re-submit a separate patch once this one is resolved one way or another. I hope you snagged the documentation update I added to your patch though. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
Greg Smith wrote: On Tue, 9 Sep 2008, Magnus Hagander wrote: (I dropped the default stuff for now, as it doesn't seem that a consensus has been reached on that topic.) This is one of the reasons I suggested keeping that one as a separate patch in the first place. The other main reason being that once it gets applied, you really want it to be two different revisions, to clearly keep them apart This means some committer is going to have to make a second pass over the same section of code and do testing there more than once, that's a waste of time I was trying to avoid. Actually, this is done all the time. Also, any standalone patch I submit right now won't apply cleanly if the source file/line patch is committed. You can always start from the patched version and use interdiff to obtain a patch difference ... If nobody cares about doing that work twice, I'll re-submit a separate patch once this one is resolved one way or another. I hope you snagged the documentation update I added to your patch though. Yeah, I did. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
Tom Lane wrote:
Alvaro Herrera [EMAIL PROTECTED] writes:
Tom Lane wrote:
Hmm, I didn't recheck after Greg's patch, but in mine, it doesn't,
because the location is saved as reset location and restored when the
variable is reset. It worked fine in all cases I tested.
Hmm. Actually, why is there a need to save and restore at all? There
can certainly never be more than one recorded config-file location per
variable. What about saying that each variable has one and only one
filename/linenumber, but whether these are relevant to the current value
is determined by whether the current value's source is S_FILE?
Hmm, this does make the patch a lot simpler. Attached. (Magnus was
visionary enough to put the correct test in the pg_settings definition.)
I also dropped the change to set_config_option, and added a new routine
to set the source file/line, as you suggested elsewhere. The only
problem is that we now have two bsearch calls for each option set in a
config file ... I don't want to change set_config_option just to be
able to return the struct config_generic for this routine's sake ...
Better ideas? Is this OK as is?
I noticed some weird things when the config files contain errors, but I
think it's outside this patch's scope.
(I dropped the default stuff for now, as it doesn't seem that a
consensus has been reached on that topic.)
--
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: doc/src/sgml/catalogs.sgml
===
RCS file: /home/alvherre/Code/cvs/pgsql/doc/src/sgml/catalogs.sgml,v
retrieving revision 2.172
diff -c -p -r2.172 catalogs.sgml
*** doc/src/sgml/catalogs.sgml 30 Jul 2008 17:05:04 - 2.172
--- doc/src/sgml/catalogs.sgml 9 Sep 2008 02:42:14 -
***
*** 6414,6419
--- 6414,6433
entryAllowed values in enum parameters (NULL for non-enum
values)/entry
/row
+ row
+ entrystructfieldsourcefile/structfield/entry
+ entrytypetext/type/entry
+ entryInput file the current value was set from (NULL for values set in
+ sources other than configuration files). Helpful when using
+ configuration include directives./entry
+ /row
+ row
+ entrystructfieldsourceline/structfield/entry
+ entrytypetext/type/entry
+ entryLine number within the sourcefile the current value was set
+ from (NULL for values set in sources other than configuration files)
+ /entry
+ /row
/tbody
/tgroup
/table
Index: src/backend/utils/misc/guc-file.l
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/misc/guc-file.l,v
retrieving revision 1.56
diff -c -p -r1.56 guc-file.l
*** src/backend/utils/misc/guc-file.l 22 Aug 2008 00:20:40 - 1.56
--- src/backend/utils/misc/guc-file.l 9 Sep 2008 02:09:28 -
*** struct name_value_pair
*** 39,44
--- 39,46
{
char *name;
char *value;
+ char *filename;
+ int sourceline;
struct name_value_pair *next;
};
*** ProcessConfigFile(GucContext context)
*** 307,314
/* If we got here all the options checked out okay, so apply them. */
for (item = head; item; item = item-next)
{
! set_config_option(item-name, item-value, context,
! PGC_S_FILE, GUC_ACTION_SET, true);
}
/* Remember when we last successfully loaded the config file. */
--- 309,320
/* If we got here all the options checked out okay, so apply them. */
for (item = head; item; item = item-next)
{
! if (set_config_option(item-name, item-value, context,
! PGC_S_FILE, GUC_ACTION_SET, true))
! {
! set_config_sourcefile(item-name, item-filename,
! item-sourceline);
! }
}
/* Remember when we last successfully loaded the config file. */
*** ParseConfigFile(const char *config_file,
*** 483,488
--- 489,496
pfree(item-value);
item-name = opt_name;
item-value = opt_value;
+ item-filename = pstrdup(config_file);
+ item-sourceline = ConfigFileLineno-1;
}
else
{
*** ParseConfigFile(const char *config_file,
*** 490,495
--- 498,505
item = palloc(sizeof *item);
item-name = opt_name;
item-value = opt_value;
+ item-filename = pstrdup(config_file);
+ item-sourceline = ConfigFileLineno-1;
item-next = *head_p;
*head_p = item;
if (*tail_p == NULL)
*** ParseConfigFile(const char *config_file,
*** 502,507
--- 512,519
item = palloc(sizeof *item);
item-name = opt_name;
item-value = opt_value;
+ item-filename = pstrdup(config_file);
+ item-sourceline = ConfigFileLineno-1;
item-next = NULL;
if (*head_p == NULL)
*head_p = item;
***
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
On Mon, 8 Sep 2008, Alvaro Herrera wrote: (I dropped the default stuff for now, as it doesn't seem that a consensus has been reached on that topic.) I have multiple GUC-related projects that are all stalled waiting for that capability to be added. The only thing there wasn't clear consensus on was exactly what the name for it should be, and there I really don't care. I made the argument for why I named it the way I did, but if it gets committed with a less friendly name (like boot_val) I won't complain. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
On Tue, 2 Sep 2008, Tom Lane wrote: How about having two new columns reset value and boot value? Like it better than default value ... It's being a bit pedantic at the expense of the user, but I don't really care that much here. I exposed the boot_val and described it in the documentation as: Default value if the parameter is not explicitly set That's the value that people care about--if they comment out a setting altogether and restart the server, what will it go back to. New admins and people playing with the config files in a tuning content aren't often using sighup in my experience, they just restart the server after changes. I'm not aware of any specific use case for exposing the reset value other than for completeness sake. Having both exposed with names that don't mean anything to new admins is making the user experience more difficult than it needs to be. That was why I just picked the more important one and named it default; that makes the case for the average user so easy they don't even need to look at the documentation. I note the ongoing GUC units debate as a reminder that a technically correct UI is usually preferred in this project to an easier to use but slightly ambiguous one, and I'm not going to argue for default further if everyone else is happy with a cryptic naming instead. The important thing is that the boot_val gets exposed somehow so tool writers can trivially present it as an option. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
Greg Smith [EMAIL PROTECTED] writes: On Tue, 2 Sep 2008, Tom Lane wrote: How about having two new columns reset value and boot value? Like it better than default value ... It's being a bit pedantic at the expense of the user, but I don't really care that much here. I exposed the boot_val and described it in the documentation as: Default value if the parameter is not explicitly set If that statement were the truth, the whole truth, and nothing but the truth, and if it didn't ignore the point about explicitly set WHERE?, I'd be fine with it. That was why I just picked the more important one and named it default; More important to whom? You are adopting a very narrow mindset, which seems to be that only DBAs look at this view. 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
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
Alvaro Herrera [EMAIL PROTECTED] writes: Greg just sent me this patch, augmenting the one I sent to add source file and line to GUC vars; Greg's patch adds a column with the default value of each var. I haven't tested, but doesn't this lose the source-location information if a setting acquired from the config file is temporarily overridden via SET (consider SET LOCAL, or a SET in a rolled-back xact)? It'll go to NULL and not come back. Since there is no possibility that any module outside GUC should ever supply a config file location, I don't think that changing the API for set_config_option is a good idea. Instead have ProcessConfigFile() call some internal function that's not used by anyone else, and let set_config_option assume that it's setting a non-file-sourced value. That'd reduce the footprint of the patch quite a bit. I dislike using the terminology default so cavalierly, because that is a fairly slippery concept in GUC. Default for whom, with respect to what? It looks like the patch actually means it to be the boot_val, but I think a lot of users would think that default refers to the reset value, ie, what their setting will be if they haven't said SET. The fact that the session default didn't necessarily come from the config file (see ALTER USER SET, ALTER DATABASE SET) complicates matters still more. *Please* use some other word than default to title that column. Also, I think that a reasonable case could be made for exposing both boot_val and reset_val in the view --- if there is a use for one, there is likely to be a use for the other. 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
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: I haven't tested, but doesn't this lose the source-location information if a setting acquired from the config file is temporarily overridden via SET (consider SET LOCAL, or a SET in a rolled-back xact)? It'll go to NULL and not come back. Hmm, I didn't recheck after Greg's patch, but in mine, it doesn't, because the location is saved as reset location and restored when the variable is reset. It worked fine in all cases I tested. Hmm. Actually, why is there a need to save and restore at all? There can certainly never be more than one recorded config-file location per variable. What about saying that each variable has one and only one filename/linenumber, but whether these are relevant to the current value is determined by whether the current value's source is S_FILE? (This would help to address one of the other things I find annoying about the patch, which is the amount of storage it eats up for N copies of what will always be the same filename in practice...) Will look into it. FWIW I think most of the callers of set_config_option are already abusing the API, because they should be calling SetConfigOption instead. Maybe this needs some cleanup. Yeah, could be. Maybe set_config_option shouldn't be declared in guc.h? Also, I think that a reasonable case could be made for exposing both boot_val and reset_val in the view --- if there is a use for one, there is likely to be a use for the other. How about having two new columns reset value and boot value? Like it better than default value ... 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
Re: [EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Greg just sent me this patch, augmenting the one I sent to add source file and line to GUC vars; Greg's patch adds a column with the default value of each var. I haven't tested, but doesn't this lose the source-location information if a setting acquired from the config file is temporarily overridden via SET (consider SET LOCAL, or a SET in a rolled-back xact)? It'll go to NULL and not come back. Hmm, I didn't recheck after Greg's patch, but in mine, it doesn't, because the location is saved as reset location and restored when the variable is reset. It worked fine in all cases I tested. Since there is no possibility that any module outside GUC should ever supply a config file location, I don't think that changing the API for set_config_option is a good idea. Instead have ProcessConfigFile() call some internal function that's not used by anyone else, and let set_config_option assume that it's setting a non-file-sourced value. That'd reduce the footprint of the patch quite a bit. Will look into it. FWIW I think most of the callers of set_config_option are already abusing the API, because they should be calling SetConfigOption instead. Maybe this needs some cleanup. Also, I think that a reasonable case could be made for exposing both boot_val and reset_val in the view --- if there is a use for one, there is likely to be a use for the other. How about having two new columns reset value and boot value? -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[EMAIL PROTECTED]: Re: [HACKERS] [patch] GUC source file and line number]
Greg just sent me this patch, augmenting the one I sent to add source file and line to GUC vars; Greg's patch adds a column with the default value of each var. I forward it to -hackers to have a public Message-Id to link to in the Commitfest page. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. ---BeginMessage--- Attached patch extends the last one Alvaro sent me to add a default_val column to pg_settings. I renumbered the new columns the sourcefile/line patch added to fit it into a logical order (the default is just after the enumvalues). That and minimizing the number of times everybody has to test in this area are why I wanted to squeeze this in at the same time. Sample that shows the sort of thing I expect tuning tools will use this for: select name,setting,default_val from pg_settings where not setting=default_val; name| setting |default_val ++--- archive_command| (disabled) | client_encoding| UTF8 | SQL_ASCII default_text_search_config | pg_catalog.english | pg_catalog.simple lc_collate | en_US.UTF-8| C lc_ctype | en_US.UTF-8| C lc_messages| en_US.UTF-8| lc_monetary| en_US.UTF-8| C lc_numeric | en_US.UTF-8| C lc_time| en_US.UTF-8| C log_timezone | US/Eastern | UNKNOWN max_fsm_pages | 204800 | 2 max_stack_depth| 2048 | 100 server_encoding| UTF8 | SQL_ASCII shared_buffers | 4096 | 1024 TimeZone | US/Eastern | UNKNOWN timezone_abbreviations | Default| UNKNOWN While there is a default for source, it's impossible to distinguish cases where someone set the value explicitly, but to the default value, unless tool writers have their own database of what the defaults are. That situation often happens when people uncomment the parameter in their postgresql.conf but don't actually change it from the default. Example: select name,setting,default_val,sourcefile,sourceline from pg_settings where name='max_connections'; name | setting | default_val | sourcefile | sourceline -+-+-+-+ max_connections | 100 | 100 | /home/gsmith/pgproject/guc/data/postgresql.conf | 61 Hope I didn't introduce any bugs in your otherwise clean patch. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MDIndex: doc/src/sgml/catalogs.sgml === RCS file: /home/gsmith/cvsrepo/pgsql/doc/src/sgml/catalogs.sgml,v retrieving revision 2.172 diff -c -r2.172 catalogs.sgml *** doc/src/sgml/catalogs.sgml 30 Jul 2008 17:05:04 - 2.172 --- doc/src/sgml/catalogs.sgml 1 Sep 2008 23:55:25 - *** *** 6414,6419 --- 6414,6436 entryAllowed values in enum parameters (NULL for non-enum values)/entry /row + row + entrystructfielddefault_val/structfield/entry + entrytypetext/type/entry + entryDefault value if the parameter is not explicitly set/entry + /row + row + entrystructfieldsourcefile/structfield/entry + entrytypetext/type/entry + entryInput file the current value was set from (if any), helpful + when using configuration include directives/entry + /row + row + entrystructfieldsourceline/structfield/entry + entrytypetext/type/entry + entryLine number within the sourcefile the current value was set + from/entry + /row /tbody /tgroup /table Index: src/backend/utils/adt/ri_triggers.c === RCS file: /home/gsmith/cvsrepo/pgsql/src/backend/utils/adt/ri_triggers.c,v retrieving revision 1.109 diff -c -r1.109 ri_triggers.c *** src/backend/utils/adt/ri_triggers.c 19 May 2008 04:14:24 - 1.109 --- src/backend/utils/adt/ri_triggers.c 1 Sep 2008 23:27:02 - *** *** 2738,2743 --- 2738,2744 snprintf(workmembuf, sizeof(workmembuf), %d, maintenance_work_mem); (void) set_config_option(work_mem, workmembuf, PGC_USERSET, PGC_S_SESSION, +NULL, 0, GUC_ACTION_LOCAL, true); if (SPI_connect() != SPI_OK_CONNECT) *** *** 2827,2832 --- 2828,2834 snprintf(workmembuf, sizeof(workmembuf), %d,
