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

Search Discussions

  • Robert Haas at Mar 31, 2011 at 8:01 pm

    On Thu, Mar 31, 2011 at 8:38 AM, Bernd Helmle 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
  • Tom Lane at Apr 2, 2011 at 10:50 pm

    Robert Haas writes:
    On Thu, Mar 31, 2011 at 8:38 AM, Bernd Helmle 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
  • Tom Lane at Apr 3, 2011 at 5:26 pm

    I wrote:
    Robert Haas <robertmhaas@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
  • Robert Haas at Apr 3, 2011 at 10:33 pm

    On Sun, Apr 3, 2011 at 1:26 PM, Tom Lane 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
  • Tom Lane at Apr 3, 2011 at 11:16 pm

    Robert Haas writes:
    On Sun, Apr 3, 2011 at 1:26 PM, Tom Lane 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
  • Tom Lane at Apr 4, 2011 at 6:41 pm

    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 lines of
    void GUC_assign_errdetail(const char *format, ...)
    The simplest implementation of this would rely on the assumption that
    assign-hook-calling isn't re-entrant, so it could just store the
    formatted string in a static variable. That seems a bit ugly, but at
    least the ugliness would be hidden in a well-defined place, where it
    could be fixed locally if the assumption ever breaks down.

    At this point I'm leaning to #3 but wondered if anyone would see it
    as either overkill or too ugly. With any of these variants, we could
    forget about my previous suggestion of adding an explicit elevel
    parameter to assign hook calls, since the hooks would no longer need
    to know the elog level to use anyway.

    Comments?

    regards, tom lane
  • Robert Haas at Apr 4, 2011 at 6:52 pm

    On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane 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 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 lines of
    void GUC_assign_errdetail(const char *format, ...)
    The simplest implementation of this would rely on the assumption that
    assign-hook-calling isn't re-entrant, so it could just store the
    formatted string in a static variable.  That seems a bit ugly, but at
    least the ugliness would be hidden in a well-defined place, where it
    could be fixed locally if the assumption ever breaks down.

    At this point I'm leaning to #3 but wondered if anyone would see it
    as either overkill or too ugly.  With any of these variants, we could
    forget about my previous suggestion of adding an explicit elevel
    parameter to assign hook calls, since the hooks would no longer need
    to know the elog level to use anyway.
    Definitely +1 for #3.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Tom Lane at Apr 4, 2011 at 6:58 pm

    Robert Haas writes:
    On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane 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
  • Robert Haas at Apr 4, 2011 at 7:09 pm

    On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane 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
  • Tom Lane at Apr 4, 2011 at 7:15 pm

    Robert Haas writes:
    On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane 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
  • Robert Haas at Apr 4, 2011 at 7:53 pm

    On Mon, Apr 4, 2011 at 3:14 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane 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
  • Tom Lane at Apr 4, 2011 at 8:57 pm

    Robert Haas 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
  • Robert Haas at Apr 4, 2011 at 9:26 pm

    On Mon, Apr 4, 2011 at 4:57 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@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
  • Tom Lane at Apr 6, 2011 at 4:14 pm

    I wrote:
    Robert Haas <robertmhaas@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.
    Attached is a WIP patch for this. It turned out to be a lot more work
    than I thought, because there are a lot more assign hooks than I'd
    remembered, and they all needed to be looked at. But I'm pretty happy
    with the results: things seem noticeably cleaner, and there are way
    fewer strange hacks like having magical things happen when timezone is
    set to "UNKNOWN". And it does fix Bernd's original complaint :-)

    The patch is not complete yet; I need to refactor some code in timezone
    abbreviation support and encoding-conversion support so that the assign
    hooks don't have to duplicate work (and risk of failure) from the check
    hooks. Also I've not touched contrib or the optional PLs yet.

    My original idea of just providing GUC_check_errdetail() to check hooks
    proved inadequate: there were several places where useful hints were
    being offered, and some where it seemed appropriate to override GUC's
    primary message string too. So now there are also GUC_check_errmsg()
    and GUC_check_errhint(). A few places were reporting SQLSTATE codes
    different from ERRCODE_INVALID_PARAMETER_VALUE; in particular the
    routines associated with transaction properties sometimes reported
    ERRCODE_ACTIVE_SQL_TRANSACTION instead. As the patch stands it doesn't
    maintain that behavior, but just reports ERRCODE_INVALID_PARAMETER_VALUE
    in all check-hook failure cases. We could preserve the old errcodes
    with another static variable and another function GUC_check_errcode(),
    but I'm not sure if it's worth the trouble. Thoughts?

    Also, I'm very seriously considering dropping the provision that says
    assign hooks can return a bool success flag, but instead having them
    return void and just saying they're not supposed to fail. The only ones
    that can ever return false at the moment are the ones for timezone
    abbrev and encoding, and as stated above that's only for lack of
    refactoring of their infrastructure. I think allowing failure just
    complicates matters for guc.c without much redeeming social benefit.

    Any comments? I'm hoping to push this through to commit today or
    tomorrow, so I can get back to fooling with collations.

    regards, tom lane
  • Kevin Grittner at Apr 4, 2011 at 12:07 pm

    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedMar 31, '11 at 12:38p
activeApr 6, '11 at 4:14p
posts16
users4
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase