Re: run GUC check hooks on RESET

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