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
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.
