On Fri, Apr 1, 2022 at 8:06 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> I think the patch can be salvaged, though. I like the concept
> of converting all the sub-day fields to microseconds immediately,
> because it avoids a host of issues, so I don't want to give that up.
> What I'm going to look into is detecting the sign-adjustment-needed
> case up front (which is easy enough, since it's looking at the
> input data not the conversion results) and then forcing the
> individual field values negative before we accumulate them into
> the pg_itm_in struct.
I took a stab at this issue and the attached patch (which would be
applied on top of your v10 patch) seems to fix the issue. Feel
free to ignore it if you're already working on a fix.
- Joe
From f43d27142a76fcbabf49e45b9457f8376744e759 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@gmail.com>
Date: Sat, 2 Apr 2022 14:42:18 -0400
Subject: [PATCH 2/2] Fix sql standard style negative semantics
---
src/backend/utils/adt/datetime.c | 107 ++++++++++++++-----------
src/test/regress/expected/interval.out | 14 ++++
src/test/regress/sql/interval.sql | 5 ++
3 files changed, 79 insertions(+), 47 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index dae90e4a9e..5842d249ab 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -50,6 +50,8 @@ static int DecodeDate(char *str, int fmask, int *tmask, bool *is2digits,
static char *AppendSeconds(char *cp, int sec, fsec_t fsec,
int precision, bool fillzeros);
static bool int64_multiply_add(int64 val, int64 multiplier, int64 *sum);
+static void AdjustForSqlStandardGlobalNegative(int64 *val, double *fval,
+ bool global_negative);
static bool AdjustFractMicroseconds(double frac, int64 scale,
struct pg_itm_in *itm_in);
static bool AdjustFractDays(double frac, int scale,
@@ -527,6 +529,19 @@ int64_multiply_add(int64 val, int64 multiplier, int64 *sum)
return true;
}
+/*
+ * Adjust values sign if SQL Standard style is being used and there's a
+ * single leading negative sign.
+ */
+static void AdjustForSqlStandardGlobalNegative(int64 *val, double *fval,
+ bool global_negative)
+{
+ if (*val > 0 && global_negative) {
+ *val = -*val;
+ *fval = -*fval;
+ }
+}
+
/*
* Multiply frac by scale (to produce microseconds) and add to itm_in->tm_usec.
* Returns true if successful, false if itm_in overflows.
@@ -3307,10 +3322,43 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
int64 val;
double fval;
+ bool global_negative = false;
+
*dtype = DTK_DELTA;
type = IGNORE_DTF;
ClearPgItmIn(itm_in);
+ /*----------
+ * The SQL standard defines the interval literal
+ * '-1 1:00:00'
+ * to mean "negative 1 days and negative 1 hours", while Postgres
+ * traditionally treats this as meaning "negative 1 days and positive
+ * 1 hours". In SQL_STANDARD intervalstyle, we apply the leading sign
+ * to all fields if there are no other explicit signs.
+ *
+ * We leave the signs alone if there are additional explicit signs.
+ * This protects us against misinterpreting postgres-style dump output,
+ * since the postgres-style output code has always put an explicit sign on
+ * all fields following a negative field. But note that SQL-spec output
+ * is ambiguous and can be misinterpreted on load! (So it's best practice
+ * to dump in postgres style, not SQL style.)
+ *----------
+ */
+ if (IntervalStyle == INTSTYLE_SQL_STANDARD && *field[0] == '-')
+ {
+ /* Check for additional explicit signs */
+ bool more_signs = false;
+ for (i = 1; i < nf; i++)
+ {
+ if (*field[i] == '-' || *field[i] == '+')
+ {
+ more_signs = true;
+ break;
+ }
+ }
+ global_negative = !more_signs;
+ }
+
/* read through list backwards to pick up units before values */
for (i = nf - 1; i >= 0; i--)
{
@@ -3447,18 +3495,21 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
switch (type)
{
case DTK_MICROSEC:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustMicroseconds(val, fval, 1, itm_in))
return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(MICROSECOND);
break;
case DTK_MILLISEC:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustMicroseconds(val, fval, 1000, itm_in))
return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(MILLISECOND);
break;
case DTK_SECOND:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustMicroseconds(val, fval, USECS_PER_SEC, itm_in))
return DTERR_FIELD_OVERFLOW;
@@ -3473,12 +3524,14 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_MINUTE:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustMicroseconds(val, fval, USECS_PER_MINUTE, itm_in))
return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(MINUTE);
break;
case DTK_HOUR:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustMicroseconds(val, fval, USECS_PER_HOUR, itm_in))
return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(HOUR);
@@ -3486,6 +3539,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_DAY:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustDays(val, 1, itm_in) ||
!AdjustFractMicroseconds(fval, USECS_PER_DAY, itm_in))
return DTERR_FIELD_OVERFLOW;
@@ -3493,6 +3547,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_WEEK:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustDays(val, 7, itm_in) ||
!AdjustFractDays(fval, 7, itm_in))
return DTERR_FIELD_OVERFLOW;
@@ -3500,6 +3555,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_MONTH:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustMonths(val, itm_in) ||
!AdjustFractDays(fval, DAYS_PER_MONTH, itm_in))
return DTERR_FIELD_OVERFLOW;
@@ -3507,6 +3563,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_YEAR:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustYears(val, 1, itm_in) ||
!AdjustFractYears(fval, 1, itm_in))
return DTERR_FIELD_OVERFLOW;
@@ -3514,6 +3571,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_DECADE:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustYears(val, 10, itm_in) ||
!AdjustFractYears(fval, 10, itm_in))
return DTERR_FIELD_OVERFLOW;
@@ -3521,6 +3579,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_CENTURY:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustYears(val, 100, itm_in) ||
!AdjustFractYears(fval, 100, itm_in))
return DTERR_FIELD_OVERFLOW;
@@ -3528,6 +3587,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_MILLENNIUM:
+ AdjustForSqlStandardGlobalNegative(&val, &fval, global_negative);
if (!AdjustYears(val, 1000, itm_in) ||
!AdjustFractYears(fval, 1000, itm_in))
return DTERR_FIELD_OVERFLOW;
@@ -3580,53 +3640,6 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
if (fmask == 0)
return DTERR_BAD_FORMAT;
- /*----------
- * The SQL standard defines the interval literal
- * '-1 1:00:00'
- * to mean "negative 1 days and negative 1 hours", while Postgres
- * traditionally treats this as meaning "negative 1 days and positive
- * 1 hours". In SQL_STANDARD intervalstyle, we apply the leading sign
- * to all fields if there are no other explicit signs.
- *
- * We leave the signs alone if there are additional explicit signs.
- * This protects us against misinterpreting postgres-style dump output,
- * since the postgres-style output code has always put an explicit sign on
- * all fields following a negative field. But note that SQL-spec output
- * is ambiguous and can be misinterpreted on load! (So it's best practice
- * to dump in postgres style, not SQL style.)
- *----------
- */
- if (IntervalStyle == INTSTYLE_SQL_STANDARD && *field[0] == '-')
- {
- /* Check for additional explicit signs */
- bool more_signs = false;
-
- for (i = 1; i < nf; i++)
- {
- if (*field[i] == '-' || *field[i] == '+')
- {
- more_signs = true;
- break;
- }
- }
-
- if (!more_signs)
- {
- /*
- * Rather than re-determining which field was field[0], just force
- * 'em all negative.
- */
- if (itm_in->tm_usec > 0)
- itm_in->tm_usec = -itm_in->tm_usec;
- if (itm_in->tm_mday > 0)
- itm_in->tm_mday = -itm_in->tm_mday;
- if (itm_in->tm_mon > 0)
- itm_in->tm_mon = -itm_in->tm_mon;
- if (itm_in->tm_year > 0)
- itm_in->tm_year = -itm_in->tm_year;
- }
- }
-
/* finally, AGO negates everything */
if (is_before)
{
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index 00ffe0e2be..ebcc672d13 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -1690,3 +1690,17 @@ SELECT INTERVAL '-2147483648 months -2147483648 days -9223372036854775808 us';
P-178956970Y-8M-2147483648DT-2562047788H-54.775808S
(1 row)
+SET IntervalStyle to postgres;
+SELECT INTERVAL '-23 hours 45 min 12.34 sec';
+ interval
+--------------
+ -22:14:47.66
+(1 row)
+
+SET IntervalStyle to sql_standard;
+SELECT INTERVAL '-23 hours 45 min 12.34 sec';
+ interval
+--------------
+ -23:45:12.34
+(1 row)
+
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index fc924d5bde..bf409df6cb 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -551,3 +551,8 @@ SET IntervalStyle TO sql_standard;
SELECT INTERVAL '-2147483648 months -2147483648 days -9223372036854775808 us';
SET IntervalStyle to iso_8601;
SELECT INTERVAL '-2147483648 months -2147483648 days -9223372036854775808 us';
+
+SET IntervalStyle to postgres;
+SELECT INTERVAL '-23 hours 45 min 12.34 sec';
+SET IntervalStyle to sql_standard;
+SELECT INTERVAL '-23 hours 45 min 12.34 sec';
--
2.25.1
