possible bug seen with -DCLOBBER_CACHE_ALWAYS and changing GUCs

Started by Jeff Davisover 14 years ago6 messagesbugs
Jump to latest
#1Jeff Davis
pgsql@j-davis.com

SQL:

set datestyle to postgres,us;
prepare stmt as select '02-01-2011'::date::text;
execute stmt;
set datestyle to postgres,euro;
execute stmt;
deallocate stmt;

The results I get with normal debug compilation are:

SET
PREPARE
    text
------------
 02-01-2011
(1 row)

SET
    text
------------
 01-02-2011
(1 row)

DEALLOCATE

But with -DCLOBBER_CACHE_ALWAYS and -DRELCACHE_FORCE_RELEASE, I get:

SET
PREPARE
     text
------------
 02-01-2011
(1 row)

SET
    text
------------
 02-01-2011
(1 row)

DEALLOCATE

Which one of those results is correct?

Regards,
Jeff Davis

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jeff Davis (#1)
Re: possible bug seen with -DCLOBBER_CACHE_ALWAYS and changing GUCs

Jeff Davis <pgsql@j-davis.com> writes:

SQL:
set datestyle to postgres,us;
prepare stmt as select '02-01-2011'::date::text;
execute stmt;
set datestyle to postgres,euro;
execute stmt;
deallocate stmt;

The results I get with normal debug compilation are:

SET
PREPARE
text
------------
02-01-2011
(1 row)

SET
text
------------
01-02-2011
(1 row)

DEALLOCATE

The result of parse analysis for that query is a stored date constant
(in a Const node) with a cast-to-text on top of it. The system is aware
that cast-date-to-text isn't immutable, so it doesn't try to fold the
cast operation. When you execute the query, it displays the date
constant using the now-current datestyle.

But with -DCLOBBER_CACHE_ALWAYS and -DRELCACHE_FORCE_RELEASE, I get:

SET
PREPARE
text
------------
02-01-2011
(1 row)

SET
text
------------
02-01-2011
(1 row)

DEALLOCATE

Which one of those results is correct?

I believe what is happening in the second case is that the query is
getting re-parse-analyzed, from scratch, and since now datestyle is
different (DMY not MDY), the date literal gets interpreted differently.
You could argue it either way as to which result is "more correct",
but I doubt we're going to try to do something about that. Best advice
is to avoid ambiguous input, or if you can't, at least avoid flipping
your datestyle on the fly.

regards, tom lane

#3Jeff Davis
pgsql@j-davis.com
In reply to: Tom Lane (#2)
Re: possible bug seen with -DCLOBBER_CACHE_ALWAYS and changing GUCs

On Wed, 2011-11-30 at 20:10 -0500, Tom Lane wrote:

I believe what is happening in the second case is that the query is
getting re-parse-analyzed, from scratch, and since now datestyle is
different (DMY not MDY), the date literal gets interpreted differently.
You could argue it either way as to which result is "more correct",
but I doubt we're going to try to do something about that. Best advice
is to avoid ambiguous input, or if you can't, at least avoid flipping
your datestyle on the fly.

I'm fine calling that "not a bug", though it appears to work in 8.3.

Regards,
Jeff Davis

#4Noah Misch
noah@leadboat.com
In reply to: Tom Lane (#2)
Re: possible bug seen with -DCLOBBER_CACHE_ALWAYS and changing GUCs

On Wed, Nov 30, 2011 at 08:10:22PM -0500, Tom Lane wrote:

Jeff Davis <pgsql@j-davis.com> writes:

SQL:
set datestyle to postgres,us;
prepare stmt as select '02-01-2011'::date::text;
execute stmt;
set datestyle to postgres,euro;
execute stmt;
deallocate stmt;

The results I get with normal debug compilation are:

SET
PREPARE
text
------------
02-01-2011
(1 row)

SET
text
------------
01-02-2011
(1 row)

DEALLOCATE

But with -DCLOBBER_CACHE_ALWAYS and -DRELCACHE_FORCE_RELEASE, I get:

SET
PREPARE
text
------------
02-01-2011
(1 row)

SET
text
------------
02-01-2011
(1 row)

DEALLOCATE

Which one of those results is correct?

I believe what is happening in the second case is that the query is
getting re-parse-analyzed, from scratch, and since now datestyle is
different (DMY not MDY), the date literal gets interpreted differently.
You could argue it either way as to which result is "more correct",
but I doubt we're going to try to do something about that. Best advice
is to avoid ambiguous input, or if you can't, at least avoid flipping
your datestyle on the fly.

One could defend consistent use of either the PREPARE-time DateStyle or the
EXECUTE-time DateStyle to interpret literals. However, using the value as of
the last RevalidateCachedQuery(), its timing independent of any GUC change, is
an implementation artifact with no redeeming value for the user.

This hazard also arises around IntervalStyle, TimeZone, sql_inheritance,
transform_null_equals, and array_nulls.

Implementation challenges aside, I'd contend for always using PREPARE-time
values during parse analysis. That's more consistent with the user-visible
consequences of changing search_path or standard_conforming_strings. That
said, I don't have in mind a cure clearly less ugly than the disease.

nm

#5Jeff Davis
pgsql@j-davis.com
In reply to: Tom Lane (#2)
Re: possible bug seen with -DCLOBBER_CACHE_ALWAYS and changing GUCs

On Wed, Nov 30, 2011 at 5:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The result of parse analysis for that query is a stored date constant
(in a Const node) with a cast-to-text on top of it.  The system is aware
that cast-date-to-text isn't immutable, so it doesn't try to fold the
cast operation.  When you execute the query, it displays the date
constant using the now-current datestyle.

Another thought: why does it execute the type input function (which is
dependent on a GUC), but not the cast?

Regards,
Jeff Davis

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jeff Davis (#5)
Re: possible bug seen with -DCLOBBER_CACHE_ALWAYS and changing GUCs

Jeff Davis <pgsql@j-davis.com> writes:

On Wed, Nov 30, 2011 at 5:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The result of parse analysis for that query is a stored date constant
(in a Const node) with a cast-to-text on top of it. The system is aware
that cast-date-to-text isn't immutable, so it doesn't try to fold the
cast operation. When you execute the query, it displays the date
constant using the now-current datestyle.

Another thought: why does it execute the type input function (which is
dependent on a GUC), but not the cast?

It's historical, see parse_coerce.c around line 210:

* XXX if the typinput function is not immutable, we really ought to
* postpone evaluation of the function call until runtime. But there
* is no way to represent a typinput function call as an expression
* tree, because C-string values are not Datums. (XXX This *is*
* possible as of 7.3, do we want to do it?)

(Of course, the parenthetical comment is a good deal newer than what
precedes it.)

However, I don't think this is the core issue. What is the core issue
is how many GUC settings we would like to save and reinstate for every
cached plan. We do that now for search_path because of the potential
security implications, but the costs of doing it for all of 'em seem
a tad daunting; not to mention that somebody is going to complain that
that's not the behavior he wanted.

regards, tom lane