Обсуждение: ALTER SYSTEM and ParseConfigFile()
Greetings, While working through the pg_file_settings patch, I came across this comment above ParseConfigFp() (which is called by ParseConfigFile()): src/backend/utils/misc/guc-file.l:603 ------------------------------------------------------* Output parameters:* head_p, tail_p: head and tail of linked listof name/value pairs** *head_p and *tail_p must be initialized to NULL before calling the outer* recursion level. Onexit, they contain a list of name-value pairs read* from the input file(s). ------------------------------------------------------ However, in 65d6e4c, ProcessConfigFile(), which isn't part of the recursion, was updated with a second call to ParseConfigFile(for the PG_AUTOCONF_FILENAME file), passing in the head and tail values which had been set by the first call. I'm a bit nervous that there might be an issue here due to how flex errors are handled and the recursion, though it mightalso be fine (but then why comment about it?). In any case, either the comment needs to be changed, or we should be passing clean NULL variables to ParseConfigFile andthen combining the results in ProcessConfigFile(). This is pretty orthogonal to the changes for pg_file_settings, so I'm going to continue working through that and hopefullyget it wrapped up soon. Thanks! Stephen
Stephen Frost <sfrost@snowman.net> writes: > Greetings, > While working through the pg_file_settings patch, I came across this > comment above ParseConfigFp() (which is called by ParseConfigFile()): > src/backend/utils/misc/guc-file.l:603 > ------------------------------------------------------ > * Output parameters: > * head_p, tail_p: head and tail of linked list of name/value pairs > * > * *head_p and *tail_p must be initialized to NULL before calling the outer > * recursion level. On exit, they contain a list of name-value pairs read > * from the input file(s). > ------------------------------------------------------ > However, in 65d6e4c, ProcessConfigFile(), which isn't part of the > recursion, was updated with a second call to ParseConfigFile (for the > PG_AUTOCONF_FILENAME file), passing in the head and tail values which > had been set by the first call. > I'm a bit nervous that there might be an issue here due to how flex > errors are handled and the recursion, though it might also be fine > (but then why comment about it?). > In any case, either the comment needs to be changed, or we should be > passing clean NULL variables to ParseConfigFile and then combining the > results in ProcessConfigFile(). I think the code is OK, but yeah, this comment should be changed to reflect the idea that the function will append entries to an existing list of name/value pairs (and thus, that head_p/tail_p are not output but in/out parameters). regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > I think the code is OK, but yeah, this comment should be changed to > reflect the idea that the function will append entries to an existing > list of name/value pairs (and thus, that head_p/tail_p are not output > but in/out parameters). I've pushed a fix for the comment to address this. Thanks! Stephen
Stephen Frost wrote: > * Tom Lane (tgl@sss.pgh.pa.us) wrote: > > I think the code is OK, but yeah, this comment should be changed to > > reflect the idea that the function will append entries to an existing > > list of name/value pairs (and thus, that head_p/tail_p are not output > > but in/out parameters). > > I've pushed a fix for the comment to address this. This open-coded list thingy is pretty odd. I wonder if it'd be nicer to replace it with ilist. (Not for 9.5, of course.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > Stephen Frost wrote: > > * Tom Lane (tgl@sss.pgh.pa.us) wrote: > > > I think the code is OK, but yeah, this comment should be changed to > > > reflect the idea that the function will append entries to an existing > > > list of name/value pairs (and thus, that head_p/tail_p are not output > > > but in/out parameters). > > > > I've pushed a fix for the comment to address this. > > This open-coded list thingy is pretty odd. I wonder if it'd be nicer to > replace it with ilist. (Not for 9.5, of course.) I have a feeling that much of the GUC machinery could be reworked; as Jim mentioned up-thread, it might be possible to use memory contexts too which would make a lot of it much cleaner, I believe. Agreed that it's not for 9.5 though. Thanks! Stephen
* Stephen Frost (sfrost@snowman.net) wrote: > I have a feeling that much of the GUC machinery could be reworked; as > Jim mentioned up-thread, it might be possible to use memory contexts too > which would make a lot of it much cleaner, I believe. Err, "on another thread", not on this one. :) And Jim Nasby, to be more specific. Thanks! Stephen