Re: No-rewrite timestamp<->timestamptz conversions

[Noah Misch]

On Tue, Feb 26, 2019 at 02:29:01PM +, 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.

Re: No-rewrite timestamp<->timestamptz conversions

[Tom Lane]

Noah Misch  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

Re: No-rewrite timestamp<->timestamptz conversions

[Noah Misch]

On Tue, Feb 26, 2019 at 10:46:29AM -0500, Tom Lane wrote:
> Noah Misch  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?

Re: No-rewrite timestamp<->timestamptz conversions

[Tom Lane]

Noah Misch  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

Re: No-rewrite timestamp<->timestamptz conversions

[Simon Riggs]

On Tue, 26 Feb 2019 at 06:14, Noah Misch  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 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?
>

Looks good, would need docs.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: No-rewrite timestamp<->timestamptz conversions

[Noah Misch]

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