Re: [gsmith@gregsmith.com: Re: [patch] GUC source file and line number]

Alvaro Herrera wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre@commandprompt.com> 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