List of "binary-compatible" data types
Folks,
From our docs:
"Adding a column with a non-null default or changing the type of an
existing column will require the entire table and indexes to be
rewritten. As an exception, if the USING clause does not change the
column contents and the old type is either binary coercible to the new
type or an unconstrained domain over the new type, a table rewrite is
not needed ..."
Which is nice, but nowhere do we present users with a set of
binary-compatible data types, even among the built-in types. I'd
happily write this up, if I knew what the binary-compatible data types
*were*.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 4 November 2013 21:58, Josh Berkus <josh@agliodbs.com> wrote:
Folks,
From our docs:
"Adding a column with a non-null default or changing the type of an
existing column will require the entire table and indexes to be
rewritten. As an exception, if the USING clause does not change the
column contents and the old type is either binary coercible to the new
type or an unconstrained domain over the new type, a table rewrite is
not needed ..."Which is nice, but nowhere do we present users with a set of
binary-compatible data types, even among the built-in types. I'd
happily write this up, if I knew what the binary-compatible data types
*were*.
You could try this:
SELECT
castsource::regtype::text,
array_agg(casttarget::regtype order by casttarget::regtype::text) casttargets
FROM pg_cast
WHERE castmethod = 'b'
GROUP BY 1
ORDER BY 1;
--
Thom
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thom,
SELECT
castsource::regtype::text,
array_agg(casttarget::regtype order by casttarget::regtype::text) casttargets
FROM pg_cast
WHERE castmethod = 'b'
GROUP BY 1
ORDER BY 1;
Are we actually covering 100% of these for ALTER COLUMN now?
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM3b065e94635d928510aab72dac2a61273ee5ed2bbfff7bfa28c5325aa2ecbc7863ba449c857b78995beda6962ead8a19@asav-1.01.com
On 11/04/2013 05:21 PM, Josh Berkus wrote:
Thom,
SELECT
castsource::regtype::text,
array_agg(casttarget::regtype order by casttarget::regtype::text) casttargets
FROM pg_cast
WHERE castmethod = 'b'
GROUP BY 1
ORDER BY 1;Are we actually covering 100% of these for ALTER COLUMN now?
Also, JSON <--> Text seems to be missing from the possible binary
conversions. That's a TODO, I suppose.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WMa3ccf6af164dc0cd4c3beeecc80a0998ac8c946d4c54b83748f4bef504f5f1a3eeae3c250efad8717d3ed07d2d9f5d98@asav-3.01.com
On Mon, Nov 04, 2013 at 05:23:36PM -0800, Josh Berkus wrote:
On 11/04/2013 05:21 PM, Josh Berkus wrote:
Thom,
SELECT
castsource::regtype::text,
array_agg(casttarget::regtype order by casttarget::regtype::text) casttargets
FROM pg_cast
WHERE castmethod = 'b'
GROUP BY 1
ORDER BY 1;Are we actually covering 100% of these for ALTER COLUMN now?
Yes; ALTER TABLE ALTER TYPE refers to the same metadata as Thom's query. If
you add to the list by issuing CREATE CAST ... WITHOUT FUNCTION, ALTER TABLE
will respect that, too.
Also, JSON <--> Text seems to be missing from the possible binary
conversions. That's a TODO, I suppose.
Only json --> text, not json <-- text. Note that you can add the cast
manually if you have an immediate need.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Noah,
Also, JSON <--> Text seems to be missing from the possible binary
conversions. That's a TODO, I suppose.Only json --> text, not json <-- text. Note that you can add the cast
manually if you have an immediate need.
Huh? Why would text --> JSON require a physical rewrite? We have to
validate it, sure, but we don't need to rewrite it.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM53acd385a4a453eb7bffcb5a7b76a4b283d43dc084666ed70ce5a95c0469df9fc6bc0a5b9d8b46b19562714ff93ff347@asav-1.01.com
On Tue, Nov 05, 2013 at 10:00:15AM -0800, Josh Berkus wrote:
Noah,
Also, JSON <--> Text seems to be missing from the possible binary
conversions. That's a TODO, I suppose.Only json --> text, not json <-- text. Note that you can add the cast
manually if you have an immediate need.Huh? Why would text --> JSON require a physical rewrite? We have to
validate it, sure, but we don't need to rewrite it.
That's all true, but the system has no concept like "this cast validates the
data, never changing it". We would first need to add metadata supporting such
a concept. On the other hand, "create cast (json as text) without function;"
leans only on concepts the system already knows.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Noah,
That's all true, but the system has no concept like "this cast validates the
data, never changing it". We would first need to add metadata supporting such
a concept. On the other hand, "create cast (json as text) without function;"
leans only on concepts the system already knows.
Yeah, I'm thinking it might be worth coming up with a solution for that
specific case. As users upgrade from 9.0 and 9.1 to 9.3, they're going
to want to convert their text columns containing JSON to columns of the
JSON type, and are going to be surprised how painful that is.
Of course, if we get binary JSON in 9.4 (Oleg?), then a binary
conversion will be required, so maybe it's a moot point.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM67bed6b0c29ad26d8d8d01d22d20c21a634b97a300156f0f2fe80e0c9f351e858fbaf666e77bf68bbcd9756b7cfe8f77@asav-3.01.com
On 2013-11-05 11:15:29 -0800, Josh Berkus wrote:
Noah,
That's all true, but the system has no concept like "this cast validates the
data, never changing it". We would first need to add metadata supporting such
a concept. On the other hand, "create cast (json as text) without function;"
leans only on concepts the system already knows.Yeah, I'm thinking it might be worth coming up with a solution for that
specific case. As users upgrade from 9.0 and 9.1 to 9.3, they're going
to want to convert their text columns containing JSON to columns of the
JSON type, and are going to be surprised how painful that is.
There's zap chance of doing anything for 9.3, this would require quite a
bit of code in tablecmds.c and that surely isn't going to happen in the
backbranches.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres,
There's zap chance of doing anything for 9.3, this would require quite a
bit of code in tablecmds.c and that surely isn't going to happen in the
backbranches.
Oh, sure, I was thinking of a workaround.
Actually, being able to separate "need to check contents" from "need to
rewrite values" could be useful for a lot of type conversions.
I'd also love some way of doing a no-rewrite conversion between
timestamp and timestamptz, based on the assumption that the original
values are UTC time. That's one I encounter a lot.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM831cef21d91f515afa4058a138722b9c6bce903e9d549115b95ad153e51268f5b9c8fd4f92d3e91eadecb8cb81bfecee@asav-3.01.comReference msg id not found: WM3b065e94635d928510aab72dac2a61273ee5ed2bbfff7bfa28c5325aa2ecbc7863ba449c857b78995beda6962ead8a19@asav-1.01.com
On Tue, Nov 05, 2013 at 05:02:58PM -0800, Josh Berkus wrote:
I'd also love some way of doing a no-rewrite conversion between
timestamp and timestamptz, based on the assumption that the original
values are UTC time. That's one I encounter a lot.
It was such a conversion that motivated me to add the no-rewrite ALTER TABLE
ALTER TYPE support in the first place. Interesting. Support for it didn't
end up in any submitted patch due to a formal problem: a protransform function
shall only consult IMMUTABLE facts, but we posit that timezone==UTC is a
STABLE observation. However, a protransform function can easily simplify the
immutable expression "tscol AT TIME ZONE 'UTC'", avoiding a rewrite. See
attached patch. Examples:
begin;
create table t (c timestamptz);
set client_min_messages = debug1;
-- rewrite: depends on timezone GUC
alter table t alter c type timestamp;
-- rewrite: depends on timezone GUC
alter table t alter c type timestamptz;
-- no rewrite: always UTC+0
alter table t alter c type timestamp using c at time zone 'UTC';
-- no rewrite: always UTC+0
alter table t alter c type timestamptz using c at time zone 'Etc/Universal';
-- rewrite: always UTC+0 in the present day, but not historically
alter table t alter c type timestamp using c at time zone 'Atlantic/Reykjavik';
-- rewrite: always UTC+0 in the present day, but not historically
alter table t alter c type timestamptz using c at time zone 'Africa/Lome';
-- no rewrite: always UTC+0
alter table t alter c type timestamp using c at time zone 'GMT';
-- rewrite: always UTC+1
alter table t alter c type timestamptz using c at time zone '1 hour'::interval;
-- no rewrite: always UTC+0
alter table t alter c type timestamp using c at time zone '0 hour'::interval;
rollback;
Attachments:
at-timezone-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 67e0cf9..723c670 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -27,6 +27,7 @@
#include "funcapi.h"
#include "libpq/pqformat.h"
#include "miscadmin.h"
+#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "parser/scansup.h"
#include "utils/array.h"
@@ -4874,6 +4875,87 @@ interval_part(PG_FUNCTION_ARGS)
}
+/* timestamp_zone_transform()
+ * If the zone argument of a timestamp_zone() or timestamptz_zone() call is a
+ * plan-time constant denoting a zone equivalent to UTC, the call will always
+ * return its second argument unchanged. Simplify the expression tree
+ * accordingly. Civil time zones almost never qualify, because jurisdictions
+ * that follow UTC today have not done so continuously.
+ */
+Datum
+timestamp_zone_transform(PG_FUNCTION_ARGS)
+{
+ Node *func_node = (Node *) PG_GETARG_POINTER(0);
+ FuncExpr *expr = (FuncExpr *) func_node;
+ Node *ret = NULL;
+ Node *zone_node;
+
+ Assert(IsA(expr, FuncExpr));
+ Assert(list_length(expr->args) == 2);
+
+ zone_node = (Node *) linitial(expr->args);
+
+ if (IsA(zone_node, Const) &&!((Const *) zone_node)->constisnull)
+ {
+ text *zone = DatumGetTextPP(((Const *) zone_node)->constvalue);
+ char tzname[TZ_STRLEN_MAX + 1];
+ char *lowzone;
+ int type,
+ abbrev_offset;
+ pg_tz *tzp;
+ bool noop = false;
+
+ /*
+ * If the timezone is forever UTC+0, the FuncExpr function call is a
+ * no-op for all possible timestamps. This passage mirrors code in
+ * timestamp_zone().
+ */
+ text_to_cstring_buffer(zone, tzname, sizeof(tzname));
+ lowzone = downcase_truncate_identifier(tzname,
+ strlen(tzname),
+ false);
+ type = DecodeTimezoneAbbrev(0, lowzone, &abbrev_offset, &tzp);
+ if (type == TZ || type == DTZ)
+ noop = (abbrev_offset == 0);
+ else if (type == DYNTZ)
+ {
+ /*
+ * An abbreviation of a single-offset timezone ought not to be
+ * configured as a DYNTZ, so don't bother checking.
+ */
+ }
+ else
+ {
+ long tzname_offset;
+
+ tzp = pg_tzset(tzname);
+ if (tzp && pg_get_timezone_offset(tzp, &tzname_offset))
+ noop = (tzname_offset == 0);
+ }
+
+ if (noop)
+ {
+ Node *timestamp = (Node *) lsecond(expr->args);
+
+ /* Strip any existing RelabelType node(s) */
+ while (timestamp && IsA(timestamp, RelabelType))
+ timestamp = (Node *) ((RelabelType *) timestamp)->arg;
+
+ /*
+ * Replace the FuncExpr with its timestamp argument, relabeled as
+ * though the function call had computed it.
+ */
+ ret = (Node *) makeRelabelType((Expr *) timestamp,
+ exprType(func_node),
+ exprTypmod(func_node),
+ exprCollation(func_node),
+ COERCE_EXPLICIT_CAST);
+ }
+ }
+
+ PG_RETURN_POINTER(ret);
+}
+
/* timestamp_zone()
* Encode timestamp type with specified time zone.
* This function is just timestamp2timestamptz() except instead of
@@ -4963,6 +5045,52 @@ timestamp_zone(PG_FUNCTION_ARGS)
PG_RETURN_TIMESTAMPTZ(result);
}
+/* timestamp_izone_transform()
+ * If we deduce at plan time that a particular timestamp_izone() or
+ * timestamptz_izone() call can only compute tz=0, the call will always return
+ * its second argument unchanged. Simplify the expression tree accordingly.
+ */
+Datum
+timestamp_izone_transform(PG_FUNCTION_ARGS)
+{
+ Node *func_node = (Node *) PG_GETARG_POINTER(0);
+ FuncExpr *expr = (FuncExpr *) func_node;
+ Node *ret = NULL;
+ Node *zone_node;
+
+ Assert(IsA(expr, FuncExpr));
+ Assert(list_length(expr->args) == 2);
+
+ zone_node = (Node *) linitial(expr->args);
+
+ if (IsA(zone_node, Const) &&!((Const *) zone_node)->constisnull)
+ {
+ Interval *zone;
+
+ zone = DatumGetIntervalP(((Const *) zone_node)->constvalue);
+ if (zone->month == 0 && zone->day == 0 && zone->time == 0)
+ {
+ Node *timestamp = (Node *) lsecond(expr->args);
+
+ /* Strip any existing RelabelType node(s) */
+ while (timestamp && IsA(timestamp, RelabelType))
+ timestamp = (Node *) ((RelabelType *) timestamp)->arg;
+
+ /*
+ * Replace the FuncExpr with its timestamp argument, relabeled as
+ * though the function call had computed it.
+ */
+ ret = (Node *) makeRelabelType((Expr *) timestamp,
+ exprType(func_node),
+ exprTypmod(func_node),
+ exprCollation(func_node),
+ COERCE_EXPLICIT_CAST);
+ }
+ }
+
+ PG_RETURN_POINTER(ret);
+}
+
/* timestamp_izone()
* Encode timestamp type with specified time interval as time zone.
*/
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 9edfdb8..0a31ac1 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -1155,7 +1155,9 @@ DATA(insert OID = 999 ( lseg_eq PGNSP PGUID 12 1 0 0 0 f f f t t f i 2 0 16
/* OIDS 1000 - 1999 */
-DATA(insert OID = 1026 ( timezone PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 1114 "1186 1184" _null_ _null_ _null_ _null_ timestamptz_izone _null_ _null_ _null_ ));
+DATA(insert OID = 3994 ( timestamp_izone_transform PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 2281 "2281" _null_ _null_ _null_ _null_ timestamp_izone_transform _null_ _null_ _null_ ));
+DESCR("transform a time zone adjustment");
+DATA(insert OID = 1026 ( timezone PGNSP PGUID 12 1 0 0 timestamp_izone_transform f f f f t f i 2 0 1114 "1186 1184" _null_ _null_ _null_ _null_ timestamptz_izone _null_ _null_ _null_ ));
DESCR("adjust timestamp to new time zone");
DATA(insert OID = 1031 ( aclitemin PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 1033 "2275" _null_ _null_ _null_ _null_ aclitemin _null_ _null_ _null_ ));
@@ -1269,7 +1271,9 @@ DATA(insert OID = 1156 ( timestamptz_ge PGNSP PGUID 12 1 0 0 0 f f f t t f i
DATA(insert OID = 1157 ( timestamptz_gt PGNSP PGUID 12 1 0 0 0 f f f t t f i 2 0 16 "1184 1184" _null_ _null_ _null_ _null_ timestamp_gt _null_ _null_ _null_ ));
DATA(insert OID = 1158 ( to_timestamp PGNSP PGUID 14 1 0 0 0 f f f f t f i 1 0 1184 "701" _null_ _null_ _null_ _null_ "select (''epoch''::pg_catalog.timestamptz + $1 * ''1 second''::pg_catalog.interval)" _null_ _null_ _null_ ));
DESCR("convert UNIX epoch to timestamptz");
-DATA(insert OID = 1159 ( timezone PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 1114 "25 1184" _null_ _null_ _null_ _null_ timestamptz_zone _null_ _null_ _null_ ));
+DATA(insert OID = 3995 ( timestamp_zone_transform PGNSP PGUID 12 1 0 0 0 f f f f t f i 1 0 2281 "2281" _null_ _null_ _null_ _null_ timestamp_zone_transform _null_ _null_ _null_ ));
+DESCR("transform a time zone adjustment");
+DATA(insert OID = 1159 ( timezone PGNSP PGUID 12 1 0 0 timestamp_zone_transform f f f f t f i 2 0 1114 "25 1184" _null_ _null_ _null_ _null_ timestamptz_zone _null_ _null_ _null_ ));
DESCR("adjust timestamp to new time zone");
DATA(insert OID = 1160 ( interval_in PGNSP PGUID 12 1 0 0 0 f f f f t f s 3 0 1186 "2275 26 23" _null_ _null_ _null_ _null_ interval_in _null_ _null_ _null_ ));
@@ -2994,9 +2998,9 @@ DESCR("date difference preserving months and years");
DATA(insert OID = 2059 ( age PGNSP PGUID 14 1 0 0 0 f f f f t f s 1 0 1186 "1114" _null_ _null_ _null_ _null_ "select pg_catalog.age(cast(current_date as timestamp without time zone), $1)" _null_ _null_ _null_ ));
DESCR("date difference from today preserving months and years");
-DATA(insert OID = 2069 ( timezone PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 1184 "25 1114" _null_ _null_ _null_ _null_ timestamp_zone _null_ _null_ _null_ ));
+DATA(insert OID = 2069 ( timezone PGNSP PGUID 12 1 0 0 timestamp_zone_transform f f f f t f i 2 0 1184 "25 1114" _null_ _null_ _null_ _null_ timestamp_zone _null_ _null_ _null_ ));
DESCR("adjust timestamp to new time zone");
-DATA(insert OID = 2070 ( timezone PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 1184 "1186 1114" _null_ _null_ _null_ _null_ timestamp_izone _null_ _null_ _null_ ));
+DATA(insert OID = 2070 ( timezone PGNSP PGUID 12 1 0 0 timestamp_izone_transform f f f f t f i 2 0 1184 "1186 1114" _null_ _null_ _null_ _null_ timestamp_izone _null_ _null_ _null_ ));
DESCR("adjust timestamp to new time zone");
DATA(insert OID = 2071 ( date_pl_interval PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 1114 "1082 1186" _null_ _null_ _null_ _null_ date_pl_interval _null_ _null_ _null_ ));
DATA(insert OID = 2072 ( date_mi_interval PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 1114 "1082 1186" _null_ _null_ _null_ _null_ date_mi_interval _null_ _null_ _null_ ));
diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
index 70118f5..530fef1 100644
--- a/src/include/utils/timestamp.h
+++ b/src/include/utils/timestamp.h
@@ -161,7 +161,9 @@ extern Datum timestamp_trunc(PG_FUNCTION_ARGS);
extern Datum interval_trunc(PG_FUNCTION_ARGS);
extern Datum timestamp_part(PG_FUNCTION_ARGS);
extern Datum interval_part(PG_FUNCTION_ARGS);
+extern Datum timestamp_zone_transform(PG_FUNCTION_ARGS);
extern Datum timestamp_zone(PG_FUNCTION_ARGS);
+extern Datum timestamp_izone_transform(PG_FUNCTION_ARGS);
extern Datum timestamp_izone(PG_FUNCTION_ARGS);
extern Datum timestamp_timestamptz(PG_FUNCTION_ARGS);
On Thu, Feb 05, 2015 at 08:36:18PM -0500, Noah Misch wrote:
On Tue, Nov 05, 2013 at 05:02:58PM -0800, Josh Berkus wrote:
I'd also love some way of doing a no-rewrite conversion between
timestamp and timestamptz, based on the assumption that the original
values are UTC time. That's one I encounter a lot.It was such a conversion that motivated me to add the no-rewrite ALTER TABLE
ALTER TYPE support in the first place. Interesting. Support for it didn't
end up in any submitted patch due to a formal problem: a protransform function
shall only consult IMMUTABLE facts, but we posit that timezone==UTC is a
STABLE observation. However, a protransform function can easily simplify the
immutable expression "tscol AT TIME ZONE 'UTC'", avoiding a rewrite. See
attached patch.
This (commit b8a18ad) ended up causing wrong EXPLAIN output and wrong
indxpath.c processing. Hence, commit c22ecc6 neutralized the optimization;
see that commit's threads for details. I pondered ways to solve those
problems, but I didn't come up with anything satisfying for EXPLAIN. (One
dead-end thought was to introduce an ExprShortcut node having "Node
*semantics" and "Node *shortcut" fields, where "semantics" is deparsed for
EXPLAIN and "shortcut" is actually evaluated. That would require teaching
piles of code about the new node type, which isn't appropriate for the benefit
in question.)
Stepping back a bit, commit b8a18ad didn't provide a great UI. I doubt folks
write queries this way spontaneously; to do so, they would have needed to
learn that such syntax enables this optimization. If I'm going to do
something more invasive, it should optimize the idiomatic "alter table t alter
timestamptzcol type timestamp". One could do that with a facility like
SupportRequestSimplify except permitted to consider STABLE facts. I suppose I
could add a volatility field to SupportRequestSimplify. So far, I can't think
of a second use case for such a facility, so instead I think
ATColumnChangeRequiresRewrite() should have a hard-wired call for
F_TIMESTAMPTZ_TIMESTAMP and F_TIMESTAMP_TIMESTAMPTZ. Patch attached. If we
find more applications of this concept, it shouldn't be hard to migrate this
logic into SupportRequestSimplify. Does anyone think that's better to do from
the start?
Thanks,
nm
Attachments:
at-timezone-v2.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 35bdb0e..74dd032 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -95,6 +95,7 @@
#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/timestamp.h"
#include "utils/typcache.h"
@@ -9634,11 +9635,15 @@ ATPrepAlterColumnType(List **wqueue,
* When the data type of a column is changed, a rewrite might not be required
* if the new type is sufficiently identical to the old one, and the USING
* clause isn't trying to insert some other value. It's safe to skip the
- * rewrite if the old type is binary coercible to the new type, or if the
- * new type is an unconstrained domain over the old type. In the case of a
- * constrained domain, we could get by with scanning the table and checking
- * the constraint rather than actually rewriting it, but we don't currently
- * try to do that.
+ * rewrite in these cases:
+ *
+ * - the old type is binary coercible to the new type
+ * - the new type is an unconstrained domain over the old type
+ * - {NEW,OLD} or {OLD,NEW} is {timestamptz,timestamp} and the timezone is UTC
+ *
+ * In the case of a constrained domain, we could get by with scanning the
+ * table and checking the constraint rather than actually rewriting it, but we
+ * don't currently try to do that.
*/
static bool
ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno)
@@ -9660,6 +9665,23 @@ ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno)
return true;
expr = (Node *) d->arg;
}
+ else if (IsA(expr, FuncExpr))
+ {
+ FuncExpr *f = (FuncExpr *) expr;
+
+ switch (f->funcid)
+ {
+ case F_TIMESTAMPTZ_TIMESTAMP:
+ case F_TIMESTAMP_TIMESTAMPTZ:
+ if (TimestampTimestampTzRequiresRewrite())
+ return true;
+ else
+ expr = linitial(f->args);
+ break;
+ default:
+ return true;
+ }
+ }
else
return true;
}
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index e0ef2f7..1b0effa 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -5168,6 +5168,23 @@ timestamp_izone(PG_FUNCTION_ARGS)
PG_RETURN_TIMESTAMPTZ(result);
} /* timestamp_izone() */
+/* TimestampTimestampTzRequiresRewrite()
+ *
+ * Returns false if the TimeZone GUC setting causes timestamp_timestamptz and
+ * timestamptz_timestamp to be no-ops, where the return value has the same
+ * bits as the argument. Since project convention is to assume a GUC changes
+ * no more often than STABLE functions change, the answer is valid that long.
+ */
+bool
+TimestampTimestampTzRequiresRewrite(void)
+{
+ long offset;
+
+ if (pg_get_timezone_offset(session_timezone, &offset) && offset == 0)
+ PG_RETURN_BOOL(false);
+ PG_RETURN_BOOL(true);
+}
+
/* timestamp_timestamptz()
* Convert local timestamp to timestamp at GMT
*/
diff --git a/src/include/utils/timestamp.h b/src/include/utils/timestamp.h
index aeb89dc..cb6bb4b 100644
--- a/src/include/utils/timestamp.h
+++ b/src/include/utils/timestamp.h
@@ -104,4 +104,6 @@ extern int date2isoweek(int year, int mon, int mday);
extern int date2isoyear(int year, int mon, int mday);
extern int date2isoyearday(int year, int mon, int mday);
+extern bool TimestampTimestampTzRequiresRewrite(void);
+
#endif /* TIMESTAMP_H */
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 0e32d5c..d0c9f9a 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -435,7 +435,7 @@ END;
$$;
create event trigger no_rewrite_allowed on table_rewrite
execute procedure test_evtrig_no_rewrite();
-create table rewriteme (id serial primary key, foo float);
+create table rewriteme (id serial primary key, foo float, bar timestamptz);
insert into rewriteme
select x * 1.001 from generate_series(1, 500) as t(x);
alter table rewriteme alter column foo type numeric;
@@ -458,6 +458,15 @@ alter table rewriteme
NOTICE: Table 'rewriteme' is being rewritten (reason = 4)
-- shouldn't trigger a table_rewrite event
alter table rewriteme alter column foo type numeric(12,4);
+begin;
+set timezone to 'UTC';
+alter table rewriteme alter column bar type timestamp;
+set timezone to '0';
+alter table rewriteme alter column bar type timestamptz;
+set timezone to 'Europe/London';
+alter table rewriteme alter column bar type timestamp; -- does rewrite
+NOTICE: Table 'rewriteme' is being rewritten (reason = 4)
+rollback;
-- typed tables are rewritten when their type changes. Don't emit table
-- name, because firing order is not stable.
CREATE OR REPLACE FUNCTION test_evtrig_no_rewrite() RETURNS event_trigger
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index f022cfa..3461686 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -329,7 +329,7 @@ $$;
create event trigger no_rewrite_allowed on table_rewrite
execute procedure test_evtrig_no_rewrite();
-create table rewriteme (id serial primary key, foo float);
+create table rewriteme (id serial primary key, foo float, bar timestamptz);
insert into rewriteme
select x * 1.001 from generate_series(1, 500) as t(x);
alter table rewriteme alter column foo type numeric;
@@ -352,6 +352,14 @@ alter table rewriteme
-- shouldn't trigger a table_rewrite event
alter table rewriteme alter column foo type numeric(12,4);
+begin;
+set timezone to 'UTC';
+alter table rewriteme alter column bar type timestamp;
+set timezone to '0';
+alter table rewriteme alter column bar type timestamptz;
+set timezone to 'Europe/London';
+alter table rewriteme alter column bar type timestamp; -- does rewrite
+rollback;
-- typed tables are rewritten when their type changes. Don't emit table
-- name, because firing order is not stable.
On Tue, 26 Feb 2019 at 06:14, Noah Misch <noah@leadboat.com> wrote:
On Thu, Feb 05, 2015 at 08:36:18PM -0500, Noah Misch wrote:
On Tue, Nov 05, 2013 at 05:02:58PM -0800, Josh Berkus wrote:
I'd also love some way of doing a no-rewrite conversion between
timestamp and timestamptz, based on the assumption that the original
values are UTC time. That's one I encounter a lot.It was such a conversion that motivated me to add the no-rewrite ALTER
TABLE
ALTER TYPE support in the first place. Interesting. Support for it
didn't
end up in any submitted patch due to a formal problem: a protransform
function
shall only consult IMMUTABLE facts, but we posit that timezone==UTC is a
STABLE observation. However, a protransform function can easilysimplify the
immutable expression "tscol AT TIME ZONE 'UTC'", avoiding a rewrite. See
attached patch.This (commit b8a18ad) ended up causing wrong EXPLAIN output and wrong
indxpath.c processing. Hence, commit c22ecc6 neutralized the optimization;
see that commit's threads for details. I pondered ways to solve those
problems, but I didn't come up with anything satisfying for EXPLAIN. (One
dead-end thought was to introduce an ExprShortcut node having "Node
*semantics" and "Node *shortcut" fields, where "semantics" is deparsed for
EXPLAIN and "shortcut" is actually evaluated. That would require teaching
piles of code about the new node type, which isn't appropriate for the
benefit
in question.)Stepping back a bit, commit b8a18ad didn't provide a great UI. I doubt
folks
write queries this way spontaneously; to do so, they would have needed to
learn that such syntax enables this optimization. If I'm going to do
something more invasive, it should optimize the idiomatic "alter table t
alter
timestamptzcol type timestamp". One could do that with a facility like
SupportRequestSimplify except permitted to consider STABLE facts. I
suppose I
could add a volatility field to SupportRequestSimplify. So far, I can't
think
of a second use case for such a facility, so instead I think
ATColumnChangeRequiresRewrite() should have a hard-wired call for
F_TIMESTAMPTZ_TIMESTAMP and F_TIMESTAMP_TIMESTAMPTZ. Patch attached. If
we
find more applications of this concept, it shouldn't be hard to migrate
this
logic into SupportRequestSimplify. Does anyone think that's better to do
from
the start?
Looks good, would need docs.
--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Noah Misch <noah@leadboat.com> writes:
Stepping back a bit, commit b8a18ad didn't provide a great UI. I doubt folks
write queries this way spontaneously; to do so, they would have needed to
learn that such syntax enables this optimization. If I'm going to do
something more invasive, it should optimize the idiomatic "alter table t alter
timestamptzcol type timestamp". One could do that with a facility like
SupportRequestSimplify except permitted to consider STABLE facts. I suppose I
could add a volatility field to SupportRequestSimplify. So far, I can't think
of a second use case for such a facility, so instead I think
ATColumnChangeRequiresRewrite() should have a hard-wired call for
F_TIMESTAMPTZ_TIMESTAMP and F_TIMESTAMP_TIMESTAMPTZ. Patch attached. If we
find more applications of this concept, it shouldn't be hard to migrate this
logic into SupportRequestSimplify. Does anyone think that's better to do from
the start?
It'd be nice to get the SupportRequestSimplify API correct from the first
release, so if there's even a slightly plausible reason for it to support
this, I'd be inclined to err in the direction of doing so. On the other
hand, if we really can't think of another use-case then a hard-wired fix
might be the best way.
One thing that we'd have to nail down a bit harder, if we're to add
something to the SupportRequestSimplify API, is exactly what the semantics
of the weaker check should be. The notion of "stable" has always been a
bit squishy, in that it's not totally clear what span of time stability
of the function result is being promised over. In the case at hand, for
instance, is it really impossible for the timezone GUC to change during
the execution of the ALTER TABLE command? You could probably break that
if you tried hard enough, though it seems unlikely that anyone would do so
accidentally.
I also kind of wonder whether this case arises often enough for us
to be expending so much effort optimizing it in the first place.
No doubt, where one lives colors one's opinion of how likely it is
that the timezone GUC is set to UTC ... but my guess is that that's
not true in very many installations.
regards, tom lane
On Tue, Feb 26, 2019 at 10:46:29AM -0500, Tom Lane wrote:
Noah Misch <noah@leadboat.com> writes:
Stepping back a bit, commit b8a18ad didn't provide a great UI. I doubt folks
write queries this way spontaneously; to do so, they would have needed to
learn that such syntax enables this optimization. If I'm going to do
something more invasive, it should optimize the idiomatic "alter table t alter
timestamptzcol type timestamp". One could do that with a facility like
SupportRequestSimplify except permitted to consider STABLE facts. I suppose I
could add a volatility field to SupportRequestSimplify. So far, I can't think
of a second use case for such a facility, so instead I think
ATColumnChangeRequiresRewrite() should have a hard-wired call for
F_TIMESTAMPTZ_TIMESTAMP and F_TIMESTAMP_TIMESTAMPTZ. Patch attached. If we
find more applications of this concept, it shouldn't be hard to migrate this
logic into SupportRequestSimplify. Does anyone think that's better to do from
the start?It'd be nice to get the SupportRequestSimplify API correct from the first
release, so if there's even a slightly plausible reason for it to support
this, I'd be inclined to err in the direction of doing so. On the other
hand, if we really can't think of another use-case then a hard-wired fix
might be the best way.
Is it problematic to add a volatility field later? Older support functions
will have needed to assume IMMUTABLE, and a simplification valid for IMMUTABLE
is valid for all volatility levels. Still, yes, it would be nice to have from
the start if we will indeed need it.
One thing that we'd have to nail down a bit harder, if we're to add
something to the SupportRequestSimplify API, is exactly what the semantics
of the weaker check should be. The notion of "stable" has always been a
bit squishy, in that it's not totally clear what span of time stability
of the function result is being promised over. In the case at hand, for
instance, is it really impossible for the timezone GUC to change during
the execution of the ALTER TABLE command? You could probably break that
if you tried hard enough, though it seems unlikely that anyone would do so
accidentally.
No, one certainly can change a GUC during ALTER TABLE execution. The STABLE
marking on current_setting is a fiction. I interpret that fiction as a signal
that a query has undefined behavior if you change GUCs mid-query, which seems
like a prudent level of effort toward such queries. Adding a volatility field
to SupportRequestSimplify does invite C-language extension code to participate
in deciding this undefined behavior, which would make it harder to verify that
we like the undefined behavior of the moment (e.g. doesn't crash). Perhaps
best not to overturn that rock?
Noah Misch <noah@leadboat.com> writes:
On Tue, Feb 26, 2019 at 10:46:29AM -0500, Tom Lane wrote:
It'd be nice to get the SupportRequestSimplify API correct from the first
release, so if there's even a slightly plausible reason for it to support
this, I'd be inclined to err in the direction of doing so.
Is it problematic to add a volatility field later?
Not hugely so, no. I'm thinking more in terms of support functions
having to pay attention to which version they're being compiled for
to know what they can do. But I suppose that's little worse than
any other feature addition we make at the C API level.
... is it really impossible for the timezone GUC to change during
the execution of the ALTER TABLE command? You could probably break that
if you tried hard enough, though it seems unlikely that anyone would do so
accidentally.
No, one certainly can change a GUC during ALTER TABLE execution. The STABLE
marking on current_setting is a fiction. I interpret that fiction as a signal
that a query has undefined behavior if you change GUCs mid-query, which seems
like a prudent level of effort toward such queries. Adding a volatility field
to SupportRequestSimplify does invite C-language extension code to participate
in deciding this undefined behavior, which would make it harder to verify that
we like the undefined behavior of the moment (e.g. doesn't crash). Perhaps
best not to overturn that rock?
Probably not, unless we can come up with more convincing use-cases for
it.
For the moment, I'm content with the approach in the patch you posted.
regards, tom lane
On Tue, Feb 26, 2019 at 02:29:01PM +0000, Simon Riggs wrote:
Looks good, would need docs.
The ALTER TABLE page just says "old type is either binary coercible to the new
type or an unconstrained domain over the new type." Avoiding rewrites by way
of a prosupport function or the at-timestamp-v2.patch approach is essentially
another way to achieve binary coercion. So far, we haven't documented the
individual data types affected. Since we don't mention e.g. varchar(x) ->
varchar(x+k) explicitly, I plan not to mention timestamp explicitly.