Thread: Re: run GUC check hooks on RESET
On Sat, Feb 11, 2012 at 1:36 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > "Kevin Grittner" wrote: > Tom Lane wrote: > >>> I agree it's a bug that you can do what Kevin's example shows. >> >> I'll look at it and see if I can pull together a patch. > > Attached. > > Basically, if a GUC has a check function, this patch causes it to be > run on a RESET just like it is on a SET, to make sure that the > resulting value is valid to set within the context. Some messages > needed adjustment. While I was there, I made cod a little more > consistent among related GUCs. > > I also added a little to the regression tests to cover this. This patch makes me a little nervous, because the existing behavior seems to have been coded for quite deliberately. Sadly, I don't see any comments explaining why the RESET case was excluded originally. On the other hand, I can't see what it would break, either. Have you gone through all the check hooks and verified that we're not violating any of their assumptions? I assume that you're thinking we'd only fix this in master? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, Feb 11, 2012 at 1:36 PM, Kevin Grittner > <Kevin.Grittner@wicourts.gov> wrote: >> "Kevin Grittner" wrote: >> Tom Lane wrote: >> >>>> I agree it's a bug that you can do what Kevin's example shows. >>> >>> I'll look at it and see if I can pull together a patch. >> >> Attached. >> >> Basically, if a GUC has a check function, this patch causes it to >> be run on a RESET just like it is on a SET, to make sure that the >> resulting value is valid to set within the context. Some >> messages needed adjustment. While I was there, I made cod a >> little more consistent among related GUCs. >> >> I also added a little to the regression tests to cover this. > > This patch makes me a little nervous, because the existing > behavior seems to have been coded for quite deliberately. It does, although I'm not clear *why* it was. I suspect it may have been based on an assumption that whatever value is in the reset_val field had to have been already determined to be good, so it was a waste of cycles to check it again -- without considering that the validity of making a change might depend on context. > Sadly, I don't see any comments explaining why the RESET case was > excluded originally. That is unfortunate. I guess it points out the value of adding a comment to point out why we would want to check these values even on a reset to a previously-used value. > On the other hand, I can't see what it would break, either. Have > you gone through all the check hooks and verified that we're not > violating any of their assumptions? I studied the code enough to be convinced that the patch as it stands can't break a check hook which only validates the value and/or changes the value to a canonical form. There appear to be 34 check hooks, and I reviewed the 10 in the variable.c file, although not at great depth. I could set aside some time this weekend to look at all of them, in depth, if you think that is warranted. I do think that a check hook would have to be doing something which is probably more appropriate for an assign hook to cause trouble, but I can't swear that that isn't happening without spending about a full day in reviewing it. > I assume that you're thinking we'd only fix this in master? Without this, I don't think it's possible for someone to enforce protection of their data through SSI in an ironclad way. So there is at least some case to be made to take it back as far as 9.1. I don't think it makes sense to take it further, because read-only was broken in other ways before 9.1, and I'm not aware of specific threats further back. On the other hand, it is a change in behavior with at least some chance to break code which is functioning as intended, so it's a pretty marginal candidate for back-patching from that point of view. I don't think a decision either way on that would be crazy. Personally I would hope to see it included in a 9.1 patch, perhaps after some "settling time" on master, but totally understand if the consensus is to just patch master. -Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Robert Haas <robertmhaas@gmail.com> wrote:
>> This patch makes me a little nervous, because the existing
>> behavior seems to have been coded for quite deliberately.
> It does, although I'm not clear *why* it was. I suspect it may have
> been based on an assumption that whatever value is in the reset_val
> field had to have been already determined to be good, so it was a
> waste of cycles to check it again -- without considering that the
> validity of making a change might depend on context.
Yes, I'm inclined to think the same, although obviously we need to
review the patch carefully. The GUC code is a bit ticklish.
The main thing I would be worried about is whether you're sure that
you have separated the RESET-as-a-command case from the cases where
we actually are rolling back to a previous state.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > The main thing I would be worried about is whether you're sure > that you have separated the RESET-as-a-command case from the cases > where we actually are rolling back to a previous state. I will double-check that, and make sure there is regression test coverage of that case. -Kevin
On Wed, Feb 15, 2012 at 4:47 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > That is unfortunate. I guess it points out the value of adding a > comment to point out why we would want to check these values even on > a reset to a previously-used value. +1 for such a comment. >> I assume that you're thinking we'd only fix this in master? > > Without this, I don't think it's possible for someone to enforce > protection of their data through SSI in an ironclad way. So there > is at least some case to be made to take it back as far as 9.1. I'm OK with that, but perhaps the only-tangentially-related changes where you swap the order of certain error messages ought to be separated out and committed only to master? That stuff doesn't seem like material for a back-patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Feb 15, 2012 at 4:47 PM, Kevin Grittner > <Kevin.Grittner@wicourts.gov> wrote: >> That is unfortunate. I guess it points out the value of adding a >> comment to point out why we would want to check these values even >> on a reset to a previously-used value. > > +1 for such a comment. Will do. >>> I assume that you're thinking we'd only fix this in master? >> >> Without this, I don't think it's possible for someone to enforce >> protection of their data through SSI in an ironclad way. So >> there is at least some case to be made to take it back as far as >> 9.1. > > I'm OK with that, but perhaps the only-tangentially-related > changes where you swap the order of certain error messages ought > to be separated out and committed only to master? That stuff > doesn't seem like material for a back-patch. Agreed. I'm not sure we want to change the message text at all in 9.1. Translations and all that. -Kevin
On Wed, Feb 15, 2012 at 5:36 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Agreed. I'm not sure we want to change the message text at all in > 9.1. Translations and all that. Agreed. I think we definitely don't want to do that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Tom Lane <tgl@sss.pgh.pa.us> wrote: > The main thing I would be worried about is whether you're sure > that you have separated the RESET-as-a-command case from the cases > where we actually are rolling back to a previous state. It looks good to me. I added a few regression tests for that. Robert Haas <robertmhaas@gmail.com> wrote: > +1 for such a comment. Added. The attached patch includes these. If it seems close, I'd be happy to come up with a version for the 9.1 branch. Basically it looks like that means omitting the changes to variable.c (which only changed message text and made the order of steps on related GUCs more consistent), and capturing a different out file for the expected directory. -Kevin