Re: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
Robert Haas wrote: On Sun, Apr 3, 2011 at 1:26 PM, Tom Lane wrote: It would probably take less than a day to flesh out this idea and make it happen, but it does seem like a rather large change for late alpha. what we're trying to avoid is committing new stuff that may require additional cleanup, not cleaning up the stuff we already did commit. Once we get to beta I'll be less enthusiastic about making changes like this +1 for fixing it, with full agreement with Robert's project management perspective on the issue. Having worked in this area a bit I definitely see the need in general, and for auto-tuning we pretty much have to do this to get it right. I think we should be edging into more auto-tuning capabilities as we figure them out, making this all the more important. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
I wrote: IMO the real problem is essentially that GUC assign hooks have two functions, checking and canonicalization of the value-to-be-stored versus executing secondary actions when an assignment is made; and there's no way to get at just the first one. So we cannot canonicalize the value first and then see if it's equal to the current setting. I think the only real fix is to change the API for assign hooks. This is a bit annoying but it's not like we haven't ever done that before. I'm thinking about splitting assign hooks into two functions, along the lines of bool check_hook (datatype *new_value, GucSource source) bool assign_hook (datatype new_value, GucSource source) After perusing the existing assign_hook functions, I have some further thoughts about this proposal. * Many of the existing assign hooks do a nontrivial amount of computation (such as catalog lookups) to derive internal values from the presented string; examples include assign_temp_tablespaces and assign_timezone. A naive implementation of the above design would require the assign_hook to repeat this computation after the check_hook already did it, which of course is undesirable. * Assign hooks that do catalog lookups need special pushups to avoid having to do such lookups while restoring a previous GUC setting during transaction abort (since lookups can't safely be performed in an aborted transaction). Up to now we've used ad-hoc techniques for each such variable, as seen for instance in assign_session_authorization. The usual idea is to modify the original string to store additional data, which then requires a custom show_hook to ensure only the original part of the string is shown to users. The messiest aspect of this is that it must be possible to reliably tell a modified string from original user input. I think that we can avoid the first problem and clean up the second problem if we do this: 1. Code guc.c so that the check_hook is only applied to original user input. When restoring a previous setting during abort (which necessarily will have been passed through the check_hook at an earlier time), only call the assign_hook. 2. Guarantee that the string pointer passed to the assign_hook is exactly what was returned by the check_hook, ie, guc.c is not allowed to duplicate or copy that string. Given these rules, a check_hook and assign_hook could cooperate to store additional data in what guc.c thinks is just a pointer to a string value, ie, there can be more data after the terminating \0. The assign_hook could work off just this additional data without ever doing a catalog lookup. No special show_hook is needed. Of course this only works for string GUC variables, but I'm finding it hard to visualize a case where a bool, int, float, or enum GUC could need a catalog lookup to interpret. We could possibly legislate that all of these are separately malloc'd to allow the same type of trick to be applied across the board, but I think that's overkill. We can just tell people they must use a string GUC if they need hidden data. This technique would need documentation of course, but at least it *would* be documented rather than ad-hoc for each variable. Another variant would be to allow the check_hook to pass back a separate void * value that could be passed on to the assign_hook, containing any necessary derived data. This is logically a bit cleaner, and would work for all types of GUC variables; but it would make things messier in guc.c since there would be an additional value to pass around. I'm not convinced it's worth that, but could be talked into it if anyone feels strongly about it. Another thing I was reminded of while perusing the code is the comment for GUC_complaint_elevel: * At some point it'd be nice to replace this with a mechanism that allows * the custom message to become the DETAIL line of guc.c's generic message. The reason we invented GUC_complaint_elevel in the first place was to avoid a change in the signatures of assign hooks. If we are making such a change, now would be the time to fix it, because we're never gonna fix it otherwise. I see a few ways we could do it: 1. Add a char **errdetail parameter to assign_hooks, which guc.c would initialize to NULL before call. If the hook stores a non-null pointer there, guc.c would include that string as errdetail. This is the least effort from guc.c's viewpoint, but will be relatively painful to use from the hook's standpoint, because it'd generally have to palloc some space, or maybe even set up a StringInfo buffer to contain the generated message. 2. Add a char *errdetail parameter to assign_hooks, which points at a local-variable buffer in the calling routine, of a well-known size (think GUC_ERRDETAIL_BUFSIZE macro in guc.h). Then hooks do something like snprintf(errdetail, GUC_ERRDETAIL_BUFSIZE, _(format), ...); to return an error detail string. 3. Create a function in guc.c for hooks to call, along the
Re: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane t...@sss.pgh.pa.us wrote: I wrote: IMO the real problem is essentially that GUC assign hooks have two functions, checking and canonicalization of the value-to-be-stored versus executing secondary actions when an assignment is made; and there's no way to get at just the first one. So we cannot canonicalize the value first and then see if it's equal to the current setting. I think the only real fix is to change the API for assign hooks. This is a bit annoying but it's not like we haven't ever done that before. I'm thinking about splitting assign hooks into two functions, along the lines of bool check_hook (datatype *new_value, GucSource source) bool assign_hook (datatype new_value, GucSource source) After perusing the existing assign_hook functions, I have some further thoughts about this proposal. * Many of the existing assign hooks do a nontrivial amount of computation (such as catalog lookups) to derive internal values from the presented string; examples include assign_temp_tablespaces and assign_timezone. A naive implementation of the above design would require the assign_hook to repeat this computation after the check_hook already did it, which of course is undesirable. * Assign hooks that do catalog lookups need special pushups to avoid having to do such lookups while restoring a previous GUC setting during transaction abort (since lookups can't safely be performed in an aborted transaction). Up to now we've used ad-hoc techniques for each such variable, as seen for instance in assign_session_authorization. The usual idea is to modify the original string to store additional data, which then requires a custom show_hook to ensure only the original part of the string is shown to users. The messiest aspect of this is that it must be possible to reliably tell a modified string from original user input. I think that we can avoid the first problem and clean up the second problem if we do this: 1. Code guc.c so that the check_hook is only applied to original user input. When restoring a previous setting during abort (which necessarily will have been passed through the check_hook at an earlier time), only call the assign_hook. 2. Guarantee that the string pointer passed to the assign_hook is exactly what was returned by the check_hook, ie, guc.c is not allowed to duplicate or copy that string. Given these rules, a check_hook and assign_hook could cooperate to store additional data in what guc.c thinks is just a pointer to a string value, ie, there can be more data after the terminating \0. The assign_hook could work off just this additional data without ever doing a catalog lookup. No special show_hook is needed. The only thing this proposal has to recommend it is that the current coding is even worse. Of course this only works for string GUC variables, but I'm finding it hard to visualize a case where a bool, int, float, or enum GUC could need a catalog lookup to interpret. We could possibly legislate that all of these are separately malloc'd to allow the same type of trick to be applied across the board, but I think that's overkill. We can just tell people they must use a string GUC if they need hidden data. This technique would need documentation of course, but at least it *would* be documented rather than ad-hoc for each variable. Another variant would be to allow the check_hook to pass back a separate void * value that could be passed on to the assign_hook, containing any necessary derived data. This is logically a bit cleaner, and would work for all types of GUC variables; but it would make things messier in guc.c since there would be an additional value to pass around. I'm not convinced it's worth that, but could be talked into it if anyone feels strongly about it. Another thing I was reminded of while perusing the code is the comment for GUC_complaint_elevel: * At some point it'd be nice to replace this with a mechanism that allows * the custom message to become the DETAIL line of guc.c's generic message. The reason we invented GUC_complaint_elevel in the first place was to avoid a change in the signatures of assign hooks. If we are making such a change, now would be the time to fix it, because we're never gonna fix it otherwise. I see a few ways we could do it: 1. Add a char **errdetail parameter to assign_hooks, which guc.c would initialize to NULL before call. If the hook stores a non-null pointer there, guc.c would include that string as errdetail. This is the least effort from guc.c's viewpoint, but will be relatively painful to use from the hook's standpoint, because it'd generally have to palloc some space, or maybe even set up a StringInfo buffer to contain the generated message. 2. Add a char *errdetail parameter to assign_hooks, which points at a local-variable buffer in the calling routine, of a well-known size (think
Re: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
Robert Haas robertmh...@gmail.com writes: On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane t...@sss.pgh.pa.us wrote: Given these rules, a check_hook and assign_hook could cooperate to store additional data in what guc.c thinks is just a pointer to a string value, ie, there can be more data after the terminating \0. The assign_hook could work off just this additional data without ever doing a catalog lookup. No special show_hook is needed. The only thing this proposal has to recommend it is that the current coding is even worse. Well, if you don't like that, do you like this one? Another variant would be to allow the check_hook to pass back a separate void * value that could be passed on to the assign_hook, containing any necessary derived data. This is logically a bit cleaner, and would work for all types of GUC variables; but it would make things messier in guc.c since there would be an additional value to pass around. I'm not convinced it's worth that, but could be talked into it if anyone feels strongly about it. If not, what do you suggest? 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: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane t...@sss.pgh.pa.us wrote: Given these rules, a check_hook and assign_hook could cooperate to store additional data in what guc.c thinks is just a pointer to a string value, ie, there can be more data after the terminating \0. The assign_hook could work off just this additional data without ever doing a catalog lookup. No special show_hook is needed. The only thing this proposal has to recommend it is that the current coding is even worse. Well, if you don't like that, do you like this one? To be clear, it's certainly an improvement over what we have now. Another variant would be to allow the check_hook to pass back a separate void * value that could be passed on to the assign_hook, containing any necessary derived data. This is logically a bit cleaner, and would work for all types of GUC variables; but it would make things messier in guc.c since there would be an additional value to pass around. I'm not convinced it's worth that, but could be talked into it if anyone feels strongly about it. I haven't really got the mental energy to think through all of this right now in detail, but I think that might be better. I think there's more kludgery here than we're going to fix in one pass, so as long as we're making improvements, I'm happy. Is there any case for using a Datum rather than a void * so people can pack a short quantity in directly without allocating memory, or are we expecting this to always be (say) a struct pointer? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
Robert Haas robertmh...@gmail.com writes: On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane t...@sss.pgh.pa.us wrote: Another variant would be to allow the check_hook to pass back a separate void * value that could be passed on to the assign_hook, containing any necessary derived data. This is logically a bit cleaner, and would work for all types of GUC variables; but it would make things messier in guc.c since there would be an additional value to pass around. I'm not convinced it's worth that, but could be talked into it if anyone feels strongly about it. I haven't really got the mental energy to think through all of this right now in detail, but I think that might be better. I think there's more kludgery here than we're going to fix in one pass, so as long as we're making improvements, I'm happy. Is there any case for using a Datum rather than a void * so people can pack a short quantity in directly without allocating memory, or are we expecting this to always be (say) a struct pointer? Well, I was intending to insist that the void* parameter point to a single malloc'd block, so that guc.c could release it when the value was no longer of interest by doing free(). If we don't say that, then we are going to need a free hook for those objects, which is surely way more notational overhead than is likely to be repaid for the occasional cases where a single OID or whatever would be sufficient info. 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: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
On Mon, Apr 4, 2011 at 3:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane t...@sss.pgh.pa.us wrote: Another variant would be to allow the check_hook to pass back a separate void * value that could be passed on to the assign_hook, containing any necessary derived data. This is logically a bit cleaner, and would work for all types of GUC variables; but it would make things messier in guc.c since there would be an additional value to pass around. I'm not convinced it's worth that, but could be talked into it if anyone feels strongly about it. I haven't really got the mental energy to think through all of this right now in detail, but I think that might be better. I think there's more kludgery here than we're going to fix in one pass, so as long as we're making improvements, I'm happy. Is there any case for using a Datum rather than a void * so people can pack a short quantity in directly without allocating memory, or are we expecting this to always be (say) a struct pointer? Well, I was intending to insist that the void* parameter point to a single malloc'd block, so that guc.c could release it when the value was no longer of interest by doing free(). If we don't say that, then we are going to need a free hook for those objects, which is surely way more notational overhead than is likely to be repaid for the occasional cases where a single OID or whatever would be sufficient info. OK. Please comment the crap out of whatever you do, or maybe even add a README. This stuff is just a bit arcane, and guideposts help a lot. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
Robert Haas robertmh...@gmail.com writes: OK. Please comment the crap out of whatever you do, or maybe even add a README. This stuff is just a bit arcane, and guideposts help a lot. We already have a README for that ;-). PFA, a patch to src/backend/utils/misc/README describing the proposed revised API. If nobody has any objections, I'll get on with making this happen. regards, tom lane diff --git a/src/backend/utils/misc/README b/src/backend/utils/misc/README index 881862a30b182a94cd71c6ed1a98dd6e6d5e51e0..221f595f8b72eb1cf48d318beb4aa8d3cb2b3411 100644 *** a/src/backend/utils/misc/README --- b/src/backend/utils/misc/README *** determining which setting is used. *** 12,68 Per-Variable Hooks -- ! Each variable known to GUC can optionally have an assign_hook and/or ! a show_hook to provide customized behavior. Assign hooks are used to ! perform validity checking on variable values (above and beyond what ! GUC can do). They are also used to update any derived state that needs ! to change when a GUC variable is set. Show hooks are used to modify ! the default SHOW display for a variable. If an assign_hook is provided, it points to a function of the signature ! bool assign_hook(newvalue, bool doit, GucSource source) ! where the type of newvalue matches the kind of variable. This function ! is called immediately before actually setting the variable's value (so it ! can look at the actual variable to determine the old value). If the ! function returns true then the assignment is completed; if it returns ! false then newvalue is considered invalid and the assignment is not ! performed. If doit is false then the function should simply check ! validity of newvalue and not change any derived state. The source parameter ! indicates where the new value came from. If it is = PGC_S_INTERACTIVE, ! then we are performing an interactive assignment (e.g., a SET command), and ! ereport(ERROR) is safe to do. But when source PGC_S_INTERACTIVE, we are ! reading a non-interactive option source, such as postgresql.conf. In this ! case the assign_hook should *not* ereport but should just return false if it ! doesn't like the newvalue. ! If an assign_hook returns false then guc.c will report a generic invalid ! value for option FOO error message. If you feel the need to provide a more ! specific error message, ereport() it using GUC_complaint_elevel(source) ! as the error level. Note that this might return either ERROR or a lower level ! such as LOG, so the ereport call might or might not return. If it does ! return, return false out of the assign_hook. ! For string variables, the signature for assign hooks is a bit different: ! const char *assign_hook(const char *newvalue, ! bool doit, ! GucSource source) ! The meanings of the parameters are the same as for the other types of GUC ! variables, but the return value is handled differently: ! NULL --- assignment fails (like returning false for other datatypes) ! newvalue --- assignment succeeds, assign the newvalue as-is ! malloc'd (not palloc'd!!!) string --- assign that value instead ! The third choice is allowed in case the assign_hook wants to return a ! canonical version of the new value. For example, the assign_hook for ! datestyle always returns a string that includes both output and input ! datestyle options, although the input might have specified only one. - Note that a string variable's assign_hook will NEVER be called with a NULL - value for newvalue, since there would be no way to distinguish success - and failure returns. If the boot_val or reset_val for a string variable - is NULL, it will just be assigned without calling the assign_hook. - Therefore, a NULL boot_val should never be used in combination with an - assign_hook that has side-effects, as the side-effects wouldn't happen - during a RESET that re-institutes the boot-time setting. If a show_hook is provided, it points to a function of the signature const char *show_hook(void) --- 12,108 Per-Variable Hooks -- ! Each variable known to GUC can optionally have a check_hook, an ! assign_hook, and/or a show_hook to provide customized behavior. ! Check hooks are used to perform validity checking on variable values ! (above and beyond what GUC can do), to compute derived settings when ! nontrivial work is needed to do that, and optionally to canonicalize ! user-supplied values. Assign hooks are used to update any derived state ! that needs to change when a GUC variable is set. Show hooks are used to ! modify the default SHOW display for a variable. ! ! ! If a check_hook is provided, it points to a function of the signature ! bool check_hook(datatype *newvalue, void **extra, GucSource source) ! The newvalue argument is of type bool *, int *, double *, or char ** ! for bool, int/enum, float, or string variables respectively. The check ! function should validate the
Re: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
On Mon, Apr 4, 2011 at 4:57 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: OK. Please comment the crap out of whatever you do, or maybe even add a README. This stuff is just a bit arcane, and guideposts help a lot. We already have a README for that ;-). PFA, a patch to src/backend/utils/misc/README describing the proposed revised API. If nobody has any objections, I'll get on with making this happen. Looks reasonable to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
I wrote: Robert Haas robertmh...@gmail.com writes: I had intended to commit Greg's patch with a show hook and an assign hook and the calculated value stored separately from the GUC variable, which I believe would avoid this is a problem, but Tom thought this way was better. Unfortunately, my knowledge of the GUC machinery is insufficient to think of a way to avoid it, other than the one Tom already rejected. Mph ... I think this shows the error of my thinking :-(. We at least need an assign hook here. Will fix, since it was my oversight to begin with. After thinking about this for awhile, I think the fundamental problem is in the is_newvalue_equal() function, which as its comment states is pretty cheesy: /* * Attempt (badly) to detect if a proposed new GUC setting is the same * as the current value. * * XXX this does not really work because it doesn't account for the * effects of canonicalization of string values by assign_hooks. */ If you hold your head at a funny angle you can see replacement of -1 with a suitable default value as a form of canonicalization, so the behavior Bernd complained of is exactly what the comment is talking about. We've not had complaints previously because is_newvalue_equal() is only used for PGC_POSTMASTER variables, and few of those have assign hooks that do any canonicalization. But it is possible to trigger the problem with unix_socket_directory, for example: try setting it to something like '/tmp/bogus/..', and you'll see that pg_ctl reload triggers a log message: LOG: parameter unix_socket_directory cannot be changed without restarting the server Robert had suggested fixing this by kluging up wal_buffers' assign and show hooks, but IIRC he never actually got that to work; I doubt it's possible to make it work in the EXEC_BACKEND case without some additional hacks to get the state to be properly replicated into child processes. Even if we could make it work, wal_buffers is not likely to be the last variable that we'll want to allow auto-tuning of. So I think we ought to address the underlying problem instead of trying to work around it in the variable-specific code for wal_buffers. IMO the real problem is essentially that GUC assign hooks have two functions, checking and canonicalization of the value-to-be-stored versus executing secondary actions when an assignment is made; and there's no way to get at just the first one. So we cannot canonicalize the value first and then see if it's equal to the current setting. I think the only real fix is to change the API for assign hooks. This is a bit annoying but it's not like we haven't ever done that before. I'm thinking about splitting assign hooks into two functions, along the lines of bool check_hook (datatype *new_value, GucSource source) bool assign_hook (datatype new_value, GucSource source) check_hook would validate the new value, and possibly change it (hence the pass-by-reference parameter). assign_hook would only be responsible for executing secondary actions needed when an assignment is done. The doit flag can go away since we'd not call the assign_hook at all unless we actually want the assignment to occur. I think most of the existing uses might only need one or the other of these hooks, but haven't gone through them yet. It might be appropriate to change the signatures some more while we're at it, in particular pass the desired elog message level explicitly instead of making hooks infer it from the source parameter. It would probably take less than a day to flesh out this idea and make it happen, but it does seem like a rather large change for late alpha. If people don't want to do this now, I suggest that we just live with the problem for 9.1. It's purely cosmetic, and easy enough to work around (just don't uncomment the 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: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
On Sun, Apr 3, 2011 at 1:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: IMO the real problem is essentially that GUC assign hooks have two functions, checking and canonicalization of the value-to-be-stored versus executing secondary actions when an assignment is made; and there's no way to get at just the first one. Yes, I think that's right. A related point is that the API for assign hooks is not consistent across all data types: string assign hooks can return a replacement value, whereas everyone else can only succeed or fail. It would probably take less than a day to flesh out this idea and make it happen, but it does seem like a rather large change for late alpha. If people don't want to do this now, I suggest that we just live with the problem for 9.1. It's purely cosmetic, and easy enough to work around (just don't uncomment the default value). I think it's a pretty ugly wart, so I'm inclined to say go ahead and fix it. I'm not sure what alpha is for if it's not cleaning up the detritus of all the stuff we've committed in the last 9 months. AIUI, what we're trying to avoid is committing new stuff that may require additional cleanup, not cleaning up the stuff we already did commit. Once we get to beta I'll be less enthusiastic about making changes like this, but I think most of the testing that will get done is still in front of us. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: GUC assign hooks (was Re: [HACKERS] wal_buffers = -1 and SIGHUP)
Robert Haas robertmh...@gmail.com writes: On Sun, Apr 3, 2011 at 1:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: IMO the real problem is essentially that GUC assign hooks have two functions, checking and canonicalization of the value-to-be-stored versus executing secondary actions when an assignment is made; and there's no way to get at just the first one. Yes, I think that's right. A related point is that the API for assign hooks is not consistent across all data types: string assign hooks can return a replacement value, whereas everyone else can only succeed or fail. Right. In the original design we only foresaw the need to canonicalize string values, so that's why it's like that. This change will make it more consistent. 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: [HACKERS] wal_buffers = -1 and SIGHUP
Robert Haas robertmh...@gmail.com writes:
On Thu, Mar 31, 2011 at 8:38 AM, Bernd Helmle maili...@oopsware.de wrote:
This might be nitpicking (or i'm currently missing something), but i
recognized that setting wal_buffers = -1 always triggers the following on
reload, even if nothing to wal_buffers had changed:
$ pg_ctl reload
LOG: received SIGHUP, reloading configuration files
LOG: parameter wal_buffers cannot be changed without restarting the
server
This only happens when you have wal_buffers set to -1.
This is a bug. The root cause is that, on startup, we do this:
if (xbuffers != XLOGbuffers)
{
snprintf(buf, sizeof(buf), %d, xbuffers);
SetConfigOption(wal_buffers, buf, PGC_POSTMASTER,
PGC_S_OVERRIDE);
}
I had intended to commit Greg's patch with a show hook and an assign
hook and the calculated value stored separately from the GUC variable,
which I believe would avoid this is a problem, but Tom thought this
way was better. Unfortunately, my knowledge of the GUC machinery is
insufficient to think of a way to avoid it, other than the one Tom
already rejected.
Mph ... I think this shows the error of my thinking :-(. We at least
need an assign hook here. Will fix, since it was my oversight to begin
with.
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
[HACKERS] wal_buffers = -1 and SIGHUP
This might be nitpicking (or i'm currently missing something), but i recognized that setting wal_buffers = -1 always triggers the following on reload, even if nothing to wal_buffers had changed: $ pg_ctl reload LOG: received SIGHUP, reloading configuration files LOG: parameter wal_buffers cannot be changed without restarting the server This only happens when you have wal_buffers set to -1. -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] wal_buffers = -1 and SIGHUP
On Thu, Mar 31, 2011 at 8:38 AM, Bernd Helmle maili...@oopsware.de wrote:
This might be nitpicking (or i'm currently missing something), but i
recognized that setting wal_buffers = -1 always triggers the following on
reload, even if nothing to wal_buffers had changed:
$ pg_ctl reload
LOG: received SIGHUP, reloading configuration files
LOG: parameter wal_buffers cannot be changed without restarting the
server
This only happens when you have wal_buffers set to -1.
This is a bug. The root cause is that, on startup, we do this:
if (xbuffers != XLOGbuffers)
{
snprintf(buf, sizeof(buf), %d, xbuffers);
SetConfigOption(wal_buffers, buf, PGC_POSTMASTER,
PGC_S_OVERRIDE);
}
I had intended to commit Greg's patch with a show hook and an assign
hook and the calculated value stored separately from the GUC variable,
which I believe would avoid this is a problem, but Tom thought this
way was better. Unfortunately, my knowledge of the GUC machinery is
insufficient to think of a way to avoid it, other than the one Tom
already rejected.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
