public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/8] Move processing of NULLs from BRIN support functions
8+ messages / 4 participants
[nested] [flat]
* [PATCH 3/8] Move processing of NULLs from BRIN support functions
@ 2020-04-02 00:56 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Tomas Vondra @ 2020-04-02 00:56 UTC (permalink / raw)
---
src/backend/access/brin/brin.c | 260 ++++++++++++++---------
src/backend/access/brin/brin_inclusion.c | 44 +---
src/backend/access/brin/brin_minmax.c | 41 +---
src/include/access/brin_internal.h | 3 +
4 files changed, 169 insertions(+), 179 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index caf7b62688..a9c44c0b82 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -35,6 +35,7 @@
#include "storage/freespace.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/datum.h"
#include "utils/index_selfuncs.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -77,7 +78,9 @@ static void form_and_insert_tuple(BrinBuildState *state);
static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a,
BrinTuple *b);
static void brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy);
-
+static bool add_values_to_range(Relation idxRel, BrinDesc *bdesc,
+ BrinMemTuple *dtup, Datum *values, bool *nulls);
+static bool check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys);
/*
* BRIN handler function: return IndexAmRoutine with access method parameters
@@ -178,7 +181,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
OffsetNumber off;
BrinTuple *brtup;
BrinMemTuple *dtup;
- int keyno;
CHECK_FOR_INTERRUPTS();
@@ -242,31 +244,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
dtup = brin_deform_tuple(bdesc, brtup, NULL);
- /*
- * Compare the key values of the new tuple to the stored index values;
- * our deformed tuple will get updated if the new tuple doesn't fit
- * the original range (note this means we can't break out of the loop
- * early). Make a note of whether this happens, so that we know to
- * insert the modified tuple later.
- */
- for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
- {
- Datum result;
- BrinValues *bval;
- FmgrInfo *addValue;
-
- bval = &dtup->bt_columns[keyno];
- addValue = index_getprocinfo(idxRel, keyno + 1,
- BRIN_PROCNUM_ADDVALUE);
- result = FunctionCall4Coll(addValue,
- idxRel->rd_indcollation[keyno],
- PointerGetDatum(bdesc),
- PointerGetDatum(bval),
- values[keyno],
- nulls[keyno]);
- /* if that returned true, we need to insert the updated tuple */
- need_insert |= DatumGetBool(result);
- }
+ need_insert = add_values_to_range(idxRel, bdesc, dtup, values, nulls);
if (!need_insert)
{
@@ -359,6 +337,7 @@ brinbeginscan(Relation r, int nkeys, int norderbys)
return scan;
}
+
/*
* Execute the index scan.
*
@@ -562,69 +541,31 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
(nkeys[attno - 1] <= scan->numberOfKeys));
/*
- * First check if there are any IS [NOT] NULL scan keys, and
- * if we're violating them. In that case we can terminate
- * early, without invoking the support function.
+ * First check if there are any IS [NOT] NULL scan keys,
+ * and if we're violating them. In that case we can
+ * terminate early, without invoking the support function.
*
* As there may be more keys, we can only detemine mismatch
* within this loop.
*/
- for (keyno = 0; (keyno < nnullkeys[attno - 1]); keyno++)
+ if (bdesc->bd_info[attno - 1]->oi_regular_nulls &&
+ !check_null_keys(bval, nullkeys[attno - 1],
+ nnullkeys[attno - 1]))
{
- ScanKey key = nullkeys[attno - 1][keyno];
-
- Assert(key->sk_attno == bval->bv_attno);
-
- /* interrupt the loop as soon as we find a mismatch */
- if (!addrange)
- break;
-
- /* handle IS NULL/IS NOT NULL tests */
- if (key->sk_flags & SK_ISNULL)
- {
- /* IS NULL scan key, but range has no NULLs */
- if (key->sk_flags & SK_SEARCHNULL)
- {
- if (!bval->bv_allnulls && !bval->bv_hasnulls)
- addrange = false;
-
- continue;
- }
-
- /*
- * For IS NOT NULL, we can only skip ranges that are
- * known to have only nulls.
- */
- if (key->sk_flags & SK_SEARCHNOTNULL)
- {
- if (bval->bv_allnulls)
- addrange = false;
-
- continue;
- }
-
- /*
- * Neither IS NULL nor IS NOT NULL was used; assume all
- * indexable operators are strict and thus return false
- * with NULL value in the scan key.
- */
- addrange = false;
- }
- }
-
- /*
- * If any of the IS [NOT] NULL keys failed, the page range as
- * a whole can't pass. So terminate the loop.
- */
- if (!addrange)
+ /*
+ * If any of the IS [NOT] NULL keys failed, the page
+ * range as a whole can't pass. So terminate the loop.
+ */
+ addrange = false;
break;
+ }
/*
- * So either there are no IS [NOT] NULL keys, or all passed. If
- * there are no regular scan keys, we're done - the page range
- * matches. If there are regular keys, but the page range is
- * marked as 'all nulls' it can't possibly pass (we're assuming
- * the operators are strict).
+ * So either there are no IS [NOT] NULL keys, or all passed.
+ * If there are no regular scan keys, we're done - the page
+ * range matches. If there are regular keys, but the page
+ * range is marked as 'all nulls' it can't possibly pass
+ * (we're assuming the operators are strict).
*/
/* No regular scan keys - page range as a whole passes. */
@@ -772,7 +713,6 @@ brinbuildCallback(Relation index,
{
BrinBuildState *state = (BrinBuildState *) brstate;
BlockNumber thisblock;
- int i;
thisblock = ItemPointerGetBlockNumber(tid);
@@ -801,25 +741,8 @@ brinbuildCallback(Relation index,
}
/* Accumulate the current tuple into the running state */
- for (i = 0; i < state->bs_bdesc->bd_tupdesc->natts; i++)
- {
- FmgrInfo *addValue;
- BrinValues *col;
- Form_pg_attribute attr = TupleDescAttr(state->bs_bdesc->bd_tupdesc, i);
-
- col = &state->bs_dtuple->bt_columns[i];
- addValue = index_getprocinfo(index, i + 1,
- BRIN_PROCNUM_ADDVALUE);
-
- /*
- * Update dtuple state, if and as necessary.
- */
- FunctionCall4Coll(addValue,
- attr->attcollation,
- PointerGetDatum(state->bs_bdesc),
- PointerGetDatum(col),
- values[i], isnull[i]);
- }
+ (void) add_values_to_range(index, state->bs_bdesc, state->bs_dtuple,
+ values, isnull);
}
/*
@@ -1598,6 +1521,39 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
FmgrInfo *unionFn;
BrinValues *col_a = &a->bt_columns[keyno];
BrinValues *col_b = &db->bt_columns[keyno];
+ BrinOpcInfo *opcinfo = bdesc->bd_info[keyno];
+
+ if (opcinfo->oi_regular_nulls)
+ {
+ /* Adjust "hasnulls". */
+ if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
+ col_a->bv_hasnulls = true;
+
+ /* If there are no values in B, there's nothing left to do. */
+ if (col_b->bv_allnulls)
+ continue;
+
+ /*
+ * Adjust "allnulls". If A doesn't have values, just copy the
+ * values from B into A, and we're done. We cannot run the
+ * operators in this case, because values in A might contain
+ * garbage. Note we already established that B contains values.
+ */
+ if (col_a->bv_allnulls)
+ {
+ int i;
+
+ col_a->bv_allnulls = false;
+
+ for (i = 0; i < opcinfo->oi_nstored; i++)
+ col_a->bv_values[i] =
+ datumCopy(col_b->bv_values[i],
+ opcinfo->oi_typcache[i]->typbyval,
+ opcinfo->oi_typcache[i]->typlen);
+
+ continue;
+ }
+ }
unionFn = index_getprocinfo(bdesc->bd_index, keyno + 1,
BRIN_PROCNUM_UNION);
@@ -1651,3 +1607,103 @@ brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy)
*/
FreeSpaceMapVacuum(idxrel);
}
+
+static bool
+add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup,
+ Datum *values, bool *nulls)
+{
+ int keyno;
+ bool modified = false;
+
+ /*
+ * Compare the key values of the new tuple to the stored index values;
+ * our deformed tuple will get updated if the new tuple doesn't fit
+ * the original range (note this means we can't break out of the loop
+ * early). Make a note of whether this happens, so that we know to
+ * insert the modified tuple later.
+ */
+ for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
+ {
+ Datum result;
+ BrinValues *bval;
+ FmgrInfo *addValue;
+
+ bval = &dtup->bt_columns[keyno];
+
+ if (bdesc->bd_info[keyno]->oi_regular_nulls && nulls[keyno])
+ {
+ /*
+ * If the new value is null, we record that we saw it if it's
+ * the first one; otherwise, there's nothing to do.
+ */
+ if (!bval->bv_hasnulls)
+ {
+ bval->bv_hasnulls = true;
+ modified = true;
+ }
+
+ continue;
+ }
+
+ addValue = index_getprocinfo(idxRel, keyno + 1,
+ BRIN_PROCNUM_ADDVALUE);
+ result = FunctionCall4Coll(addValue,
+ idxRel->rd_indcollation[keyno],
+ PointerGetDatum(bdesc),
+ PointerGetDatum(bval),
+ values[keyno],
+ nulls[keyno]);
+ /* if that returned true, we need to insert the updated tuple */
+ modified |= DatumGetBool(result);
+ }
+
+ return modified;
+}
+
+static bool
+check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys)
+{
+ int keyno;
+
+ /*
+ * First check if there are any IS [NOT] NULL scan keys, and if we're
+ * violating them.
+ */
+ for (keyno = 0; keyno < nnullkeys; keyno++)
+ {
+ ScanKey key = nullkeys[keyno];
+
+ Assert(key->sk_attno == bval->bv_attno);
+
+ /* Handle only IS NULL/IS NOT NULL tests */
+ if (!(key->sk_flags & SK_ISNULL))
+ continue;
+
+ if (key->sk_flags & SK_SEARCHNULL)
+ {
+ /* IS NULL scan key, but range has no NULLs */
+ if (!bval->bv_allnulls && !bval->bv_hasnulls)
+ return false;
+ }
+ else if (key->sk_flags & SK_SEARCHNOTNULL)
+ {
+ /*
+ * For IS NOT NULL, we can only skip ranges that are known to
+ * have only nulls.
+ */
+ if (bval->bv_allnulls)
+ return false;
+ }
+ else
+ {
+ /*
+ * Neither IS NULL nor IS NOT NULL was used; assume all indexable
+ * operators are strict and thus return false with NULL value in
+ * the scan key.
+ */
+ return false;
+ }
+ }
+
+ return true;
+}
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 22edc6b46f..59503b6f68 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -110,6 +110,7 @@ brin_inclusion_opcinfo(PG_FUNCTION_ARGS)
*/
result = palloc0(MAXALIGN(SizeofBrinOpcInfo(3)) + sizeof(InclusionOpaque));
result->oi_nstored = 3;
+ result->oi_regular_nulls = true;
result->oi_opaque = (InclusionOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(3));
@@ -141,7 +142,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
Datum newval = PG_GETARG_DATUM(2);
- bool isnull = PG_GETARG_BOOL(3);
+ bool isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_BOOL(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *finfo;
Datum result;
@@ -149,18 +150,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
AttrNumber attno;
Form_pg_attribute attr;
- /*
- * If the new value is null, we record that we saw it if it's the first
- * one; otherwise, there's nothing to do.
- */
- if (isnull)
- {
- if (column->bv_hasnulls)
- PG_RETURN_BOOL(false);
-
- column->bv_hasnulls = true;
- PG_RETURN_BOOL(true);
- }
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -510,37 +500,11 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
Datum result;
Assert(col_a->bv_attno == col_b->bv_attno);
-
- /* Adjust "hasnulls". */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
-
- /* If there are no values in B, there's nothing left to do. */
- if (col_b->bv_allnulls)
- PG_RETURN_VOID();
+ Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
attno = col_a->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
- /*
- * Adjust "allnulls". If A doesn't have values, just copy the values from
- * B into A, and we're done. We cannot run the operators in this case,
- * because values in A might contain garbage. Note we already established
- * that B contains values.
- */
- if (col_a->bv_allnulls)
- {
- col_a->bv_allnulls = false;
- col_a->bv_values[INCLUSION_UNION] =
- datumCopy(col_b->bv_values[INCLUSION_UNION],
- attr->attbyval, attr->attlen);
- col_a->bv_values[INCLUSION_UNMERGEABLE] =
- col_b->bv_values[INCLUSION_UNMERGEABLE];
- col_a->bv_values[INCLUSION_CONTAINS_EMPTY] =
- col_b->bv_values[INCLUSION_CONTAINS_EMPTY];
- PG_RETURN_VOID();
- }
-
/* If B includes empty elements, mark A similarly, if needed. */
if (!DatumGetBool(col_a->bv_values[INCLUSION_CONTAINS_EMPTY]) &&
DatumGetBool(col_b->bv_values[INCLUSION_CONTAINS_EMPTY]))
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 7a7bd21cec..8882eec12c 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -48,6 +48,7 @@ brin_minmax_opcinfo(PG_FUNCTION_ARGS)
result = palloc0(MAXALIGN(SizeofBrinOpcInfo(2)) +
sizeof(MinmaxOpaque));
result->oi_nstored = 2;
+ result->oi_regular_nulls = true;
result->oi_opaque = (MinmaxOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(2));
result->oi_typcache[0] = result->oi_typcache[1] =
@@ -69,7 +70,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
Datum newval = PG_GETARG_DATUM(2);
- bool isnull = PG_GETARG_DATUM(3);
+ bool isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_DATUM(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *cmpFn;
Datum compar;
@@ -77,18 +78,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
Form_pg_attribute attr;
AttrNumber attno;
- /*
- * If the new value is null, we record that we saw it if it's the first
- * one; otherwise, there's nothing to do.
- */
- if (isnull)
- {
- if (column->bv_hasnulls)
- PG_RETURN_BOOL(false);
-
- column->bv_hasnulls = true;
- PG_RETURN_BOOL(true);
- }
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -245,34 +235,11 @@ brin_minmax_union(PG_FUNCTION_ARGS)
bool needsadj;
Assert(col_a->bv_attno == col_b->bv_attno);
-
- /* Adjust "hasnulls" */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
-
- /* If there are no values in B, there's nothing left to do */
- if (col_b->bv_allnulls)
- PG_RETURN_VOID();
+ Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
attno = col_a->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
- /*
- * Adjust "allnulls". If A doesn't have values, just copy the values from
- * B into A, and we're done. We cannot run the operators in this case,
- * because values in A might contain garbage. Note we already established
- * that B contains values.
- */
- if (col_a->bv_allnulls)
- {
- col_a->bv_allnulls = false;
- col_a->bv_values[0] = datumCopy(col_b->bv_values[0],
- attr->attbyval, attr->attlen);
- col_a->bv_values[1] = datumCopy(col_b->bv_values[1],
- attr->attbyval, attr->attlen);
- PG_RETURN_VOID();
- }
-
/* Adjust minimum, if B's min is less than A's min */
finfo = minmax_get_strategy_procinfo(bdesc, attno, attr->atttypid,
BTLessStrategyNumber);
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 9ffc9100c0..7c4f3da0a0 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -27,6 +27,9 @@ typedef struct BrinOpcInfo
/* Number of columns stored in an index column of this opclass */
uint16 oi_nstored;
+ /* Regular processing of NULLs in BrinValues? */
+ bool oi_regular_nulls;
+
/* Opaque pointer for the opclass' private use */
void *oi_opaque;
--
2.25.4
--jsivyprvf2oxfjz3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename="0004-BRIN-bloom-indexes-20200807.patch"
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: date_trunc invalid units with infinite value
@ 2024-12-05 05:34 Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2024-12-05 05:34 UTC (permalink / raw)
To: Joseph Koshakow <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Sun, Dec 01, 2024 at 03:09:17PM -0500, Joseph Koshakow wrote:
> When looking at the date_trunc function, I noticed that it wasn't
> rejecting invalid units if the value was infinite. It's slightly
> annoying to fix this, but we already do something very similar for
> date_part. I have attached a patch that would fix this issue, let me
> know if you think it's worth pursuing.
I agree that it is inconsistent that we allow infinite values to go
through this function call even for fields that are not listed as
supported by the documentation. So, yes, I think that what you are
doing the right thing by applying the check based on the units
supported, but I also doubt that it is something that we could
backpatch as it would cause queries to work now to suddenly break.
Thoughts and comments from others are welcome.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: date_trunc invalid units with infinite value
@ 2024-12-24 07:33 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2024-12-24 07:33 UTC (permalink / raw)
To: Joseph Koshakow <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Thu, Dec 05, 2024 at 02:34:41PM +0900, Michael Paquier wrote:
> I agree that it is inconsistent that we allow infinite values to go
> through this function call even for fields that are not listed as
> supported by the documentation. So, yes, I think that what you are
> doing the right thing by applying the check based on the units
> supported, but I also doubt that it is something that we could
> backpatch as it would cause queries to work now to suddenly break.
>
> Thoughts and comments from others are welcome.
Hearing nothing, I have looked at this patch again and I think that
I'm OK with your proposal. While the discrepancy is annoying for
back-branches, this causes a slight change of behavior, so I have no
backpatch in mind.
I am planning to get this one applied around the end of this week on
Friday for HEAD, that should be enough if there are comments and/or
objections.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: date_trunc invalid units with infinite value
@ 2024-12-27 04:42 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2024-12-27 04:42 UTC (permalink / raw)
To: Joseph Koshakow <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Tue, Dec 24, 2024 at 04:33:47PM +0900, Michael Paquier wrote:
> I am planning to get this one applied around the end of this week on
> Friday for HEAD, that should be enough if there are comments and/or
> objections.
And done for now. If there are any remarks and/or objections, of
course feel free.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: date_trunc invalid units with infinite value
@ 2025-08-06 11:07 Peter Eisentraut <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2025-08-06 11:07 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Joseph Koshakow <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 27.12.24 05:42, Michael Paquier wrote:
> On Tue, Dec 24, 2024 at 04:33:47PM +0900, Michael Paquier wrote:
>> I am planning to get this one applied around the end of this week on
>> Friday for HEAD, that should be enough if there are comments and/or
>> objections.
>
> And done for now. If there are any remarks and/or objections, of
> course feel free.
It turned out this had a bug, and also the newly added test cases didn't
actually cover the new code, otherwise this would have shown up.
Please review the attached patches with additional test cases and the fix.
See also [0] for further context:
[0]:
https://www.postgresql.org/message-id/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.org
From 11eb6931c0903ebac40efd95cda201f341bec59b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Wed, 6 Aug 2025 12:20:44 +0200
Subject: [PATCH 1/2] Improve test coverage of date_trunc() on infinity
Commit d85ce012f99 added new error handling code to date_trunc() of
timestamp, timestamptz, and interval with infinite values. But the
new test cases added by that commit did not actually test the new
code, since they used the "not recognized" unit 'ago' but not a "not
supported" unit, which is what the new code was dealing with. This
patch adds some test cases to cover the new code. I'm using
'timezone' as the not supported unit.
---
src/test/regress/expected/timestamp.out | 10 ++++++++++
src/test/regress/expected/timestamptz.out | 12 ++++++++++++
src/test/regress/sql/timestamp.sql | 4 +++-
src/test/regress/sql/timestamptz.sql | 6 ++++--
4 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index 6aaa19c8f4e..14a9f5b56a6 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -591,6 +591,16 @@ SELECT date_trunc( 'week', timestamp '2004-02-29 15:44:17.71393' ) AS week_trunc
Mon Feb 23 00:00:00 2004
(1 row)
+SELECT date_trunc( 'week', timestamp 'infinity' ) AS inf_trunc;
+ inf_trunc
+-----------
+ infinity
+(1 row)
+
+SELECT date_trunc( 'timezone', timestamp '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+ERROR: unit "timezone" not supported for type timestamp without time zone
+SELECT date_trunc( 'timezone', timestamp 'infinity' ) AS notsupp_inf_trunc;
+ERROR: unit "timezone" not supported for type timestamp without time zone
SELECT date_trunc( 'ago', timestamp 'infinity' ) AS invalid_trunc;
ERROR: unit "ago" not recognized for type timestamp without time zone
-- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index 2a69953ff25..953656affa4 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -760,6 +760,16 @@ SELECT date_trunc( 'week', timestamp with time zone '2004-02-29 15:44:17.71393'
Mon Feb 23 00:00:00 2004 PST
(1 row)
+SELECT date_trunc( 'week', timestamp with time zone 'infinity' ) AS inf_trunc;
+ inf_trunc
+-----------
+ infinity
+(1 row)
+
+SELECT date_trunc( 'timezone', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+ERROR: unit "timezone" not supported for type timestamp with time zone
+SELECT date_trunc( 'timezone', timestamp with time zone 'infinity' ) AS notsupp_inf_trunc;
+ERROR: unit "timezone" not supported for type timestamp with time zone
SELECT date_trunc( 'ago', timestamp with time zone 'infinity' ) AS invalid_trunc;
ERROR: unit "ago" not recognized for type timestamp with time zone
SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'Australia/Sydney') as sydney_trunc; -- zone name
@@ -780,6 +790,8 @@ SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'VET
Thu Feb 15 20:00:00 2001 PST
(1 row)
+SELECT date_trunc('timezone', timestamp with time zone 'infinity', 'GMT') AS notsupp_zone_trunc;
+ERROR: unit "timezone" not supported for type timestamp with time zone
SELECT date_trunc('ago', timestamp with time zone 'infinity', 'GMT') AS invalid_zone_trunc;
ERROR: unit "ago" not recognized for type timestamp with time zone
-- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index 55f80530ea0..313757ed041 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -175,7 +175,9 @@ CREATE TABLE TIMESTAMP_TBL (d1 timestamp(2) without time zone);
FROM TIMESTAMP_TBL WHERE d1 BETWEEN '1902-01-01' AND '2038-01-01';
SELECT date_trunc( 'week', timestamp '2004-02-29 15:44:17.71393' ) AS week_trunc;
-
+SELECT date_trunc( 'week', timestamp 'infinity' ) AS inf_trunc;
+SELECT date_trunc( 'timezone', timestamp '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+SELECT date_trunc( 'timezone', timestamp 'infinity' ) AS notsupp_inf_trunc;
SELECT date_trunc( 'ago', timestamp 'infinity' ) AS invalid_trunc;
-- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql
index caca3123f13..e01618907e0 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -217,15 +217,17 @@ CREATE TABLE TIMESTAMPTZ_TBL (d1 timestamp(2) with time zone);
FROM TIMESTAMPTZ_TBL WHERE d1 BETWEEN '1902-01-01' AND '2038-01-01';
SELECT date_trunc( 'week', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS week_trunc;
+SELECT date_trunc( 'week', timestamp with time zone 'infinity' ) AS inf_trunc;
+SELECT date_trunc( 'timezone', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+SELECT date_trunc( 'timezone', timestamp with time zone 'infinity' ) AS notsupp_inf_trunc;
SELECT date_trunc( 'ago', timestamp with time zone 'infinity' ) AS invalid_trunc;
SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'Australia/Sydney') as sydney_trunc; -- zone name
SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'GMT') as gmt_trunc; -- fixed-offset abbreviation
SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'VET') as vet_trunc; -- variable-offset abbreviation
+SELECT date_trunc('timezone', timestamp with time zone 'infinity', 'GMT') AS notsupp_zone_trunc;
SELECT date_trunc('ago', timestamp with time zone 'infinity', 'GMT') AS invalid_zone_trunc;
-
-
-- verify date_bin behaves the same as date_trunc for relevant intervals
SELECT
str,
--
2.50.1
From 4f9e5dde59efc00c682f14a3047fd432c87f4209 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Wed, 6 Aug 2025 12:37:41 +0200
Subject: [PATCH 2/2] Fix incorrect Datum conversion in
timestamptz_trunc_internal()
It used PG_RETURN_TIMESTAMPTZ(), but the return type is TimestampTz,
not Datum. On 64-bit systems, there is no effect, since this just
ends up casting 64-bit integers back and forth. But on 32-bit
systems, timestamptz is pass-by-reference, and so
PG_RETURN_TIMESTAMPTZ() allocates new memory and returns the address,
but the caller will try to interpret this as a timestamp value. The
effect is that date_trunc(..., 'infinity'::timestamptz) will return
random values (instead of the correct return value 'infinity').
The fix is to use a straight "return" call.
Commit d85ce012f99 added this code but provided tests that did not
cover this correctly. This was fixed in the previous patch.
Reviewed-by: Tom Lane <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.org
---
src/backend/utils/adt/timestamp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 25cff56c3d0..e640b48205b 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -4954,7 +4954,7 @@ timestamptz_trunc_internal(text *units, TimestampTz timestamp, pg_tz *tzp)
case DTK_SECOND:
case DTK_MILLISEC:
case DTK_MICROSEC:
- PG_RETURN_TIMESTAMPTZ(timestamp);
+ return timestamp;
break;
default:
--
2.50.1
Attachments:
[text/plain] 0001-Improve-test-coverage-of-date_trunc-on-infinity.patch (5.8K, ../../[email protected]/2-0001-Improve-test-coverage-of-date_trunc-on-infinity.patch)
download | inline diff:
From 11eb6931c0903ebac40efd95cda201f341bec59b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Wed, 6 Aug 2025 12:20:44 +0200
Subject: [PATCH 1/2] Improve test coverage of date_trunc() on infinity
Commit d85ce012f99 added new error handling code to date_trunc() of
timestamp, timestamptz, and interval with infinite values. But the
new test cases added by that commit did not actually test the new
code, since they used the "not recognized" unit 'ago' but not a "not
supported" unit, which is what the new code was dealing with. This
patch adds some test cases to cover the new code. I'm using
'timezone' as the not supported unit.
---
src/test/regress/expected/timestamp.out | 10 ++++++++++
src/test/regress/expected/timestamptz.out | 12 ++++++++++++
src/test/regress/sql/timestamp.sql | 4 +++-
src/test/regress/sql/timestamptz.sql | 6 ++++--
4 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index 6aaa19c8f4e..14a9f5b56a6 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -591,6 +591,16 @@ SELECT date_trunc( 'week', timestamp '2004-02-29 15:44:17.71393' ) AS week_trunc
Mon Feb 23 00:00:00 2004
(1 row)
+SELECT date_trunc( 'week', timestamp 'infinity' ) AS inf_trunc;
+ inf_trunc
+-----------
+ infinity
+(1 row)
+
+SELECT date_trunc( 'timezone', timestamp '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+ERROR: unit "timezone" not supported for type timestamp without time zone
+SELECT date_trunc( 'timezone', timestamp 'infinity' ) AS notsupp_inf_trunc;
+ERROR: unit "timezone" not supported for type timestamp without time zone
SELECT date_trunc( 'ago', timestamp 'infinity' ) AS invalid_trunc;
ERROR: unit "ago" not recognized for type timestamp without time zone
-- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index 2a69953ff25..953656affa4 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -760,6 +760,16 @@ SELECT date_trunc( 'week', timestamp with time zone '2004-02-29 15:44:17.71393'
Mon Feb 23 00:00:00 2004 PST
(1 row)
+SELECT date_trunc( 'week', timestamp with time zone 'infinity' ) AS inf_trunc;
+ inf_trunc
+-----------
+ infinity
+(1 row)
+
+SELECT date_trunc( 'timezone', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+ERROR: unit "timezone" not supported for type timestamp with time zone
+SELECT date_trunc( 'timezone', timestamp with time zone 'infinity' ) AS notsupp_inf_trunc;
+ERROR: unit "timezone" not supported for type timestamp with time zone
SELECT date_trunc( 'ago', timestamp with time zone 'infinity' ) AS invalid_trunc;
ERROR: unit "ago" not recognized for type timestamp with time zone
SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'Australia/Sydney') as sydney_trunc; -- zone name
@@ -780,6 +790,8 @@ SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'VET
Thu Feb 15 20:00:00 2001 PST
(1 row)
+SELECT date_trunc('timezone', timestamp with time zone 'infinity', 'GMT') AS notsupp_zone_trunc;
+ERROR: unit "timezone" not supported for type timestamp with time zone
SELECT date_trunc('ago', timestamp with time zone 'infinity', 'GMT') AS invalid_zone_trunc;
ERROR: unit "ago" not recognized for type timestamp with time zone
-- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index 55f80530ea0..313757ed041 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -175,7 +175,9 @@ CREATE TABLE TIMESTAMP_TBL (d1 timestamp(2) without time zone);
FROM TIMESTAMP_TBL WHERE d1 BETWEEN '1902-01-01' AND '2038-01-01';
SELECT date_trunc( 'week', timestamp '2004-02-29 15:44:17.71393' ) AS week_trunc;
-
+SELECT date_trunc( 'week', timestamp 'infinity' ) AS inf_trunc;
+SELECT date_trunc( 'timezone', timestamp '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+SELECT date_trunc( 'timezone', timestamp 'infinity' ) AS notsupp_inf_trunc;
SELECT date_trunc( 'ago', timestamp 'infinity' ) AS invalid_trunc;
-- verify date_bin behaves the same as date_trunc for relevant intervals
diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql
index caca3123f13..e01618907e0 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -217,15 +217,17 @@ CREATE TABLE TIMESTAMPTZ_TBL (d1 timestamp(2) with time zone);
FROM TIMESTAMPTZ_TBL WHERE d1 BETWEEN '1902-01-01' AND '2038-01-01';
SELECT date_trunc( 'week', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS week_trunc;
+SELECT date_trunc( 'week', timestamp with time zone 'infinity' ) AS inf_trunc;
+SELECT date_trunc( 'timezone', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS notsupp_trunc;
+SELECT date_trunc( 'timezone', timestamp with time zone 'infinity' ) AS notsupp_inf_trunc;
SELECT date_trunc( 'ago', timestamp with time zone 'infinity' ) AS invalid_trunc;
SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'Australia/Sydney') as sydney_trunc; -- zone name
SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'GMT') as gmt_trunc; -- fixed-offset abbreviation
SELECT date_trunc('day', timestamp with time zone '2001-02-16 20:38:40+00', 'VET') as vet_trunc; -- variable-offset abbreviation
+SELECT date_trunc('timezone', timestamp with time zone 'infinity', 'GMT') AS notsupp_zone_trunc;
SELECT date_trunc('ago', timestamp with time zone 'infinity', 'GMT') AS invalid_zone_trunc;
-
-
-- verify date_bin behaves the same as date_trunc for relevant intervals
SELECT
str,
--
2.50.1
[text/plain] 0002-Fix-incorrect-Datum-conversion-in-timestamptz_trunc_.patch (1.6K, ../../[email protected]/3-0002-Fix-incorrect-Datum-conversion-in-timestamptz_trunc_.patch)
download | inline diff:
From 4f9e5dde59efc00c682f14a3047fd432c87f4209 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Wed, 6 Aug 2025 12:37:41 +0200
Subject: [PATCH 2/2] Fix incorrect Datum conversion in
timestamptz_trunc_internal()
It used PG_RETURN_TIMESTAMPTZ(), but the return type is TimestampTz,
not Datum. On 64-bit systems, there is no effect, since this just
ends up casting 64-bit integers back and forth. But on 32-bit
systems, timestamptz is pass-by-reference, and so
PG_RETURN_TIMESTAMPTZ() allocates new memory and returns the address,
but the caller will try to interpret this as a timestamp value. The
effect is that date_trunc(..., 'infinity'::timestamptz) will return
random values (instead of the correct return value 'infinity').
The fix is to use a straight "return" call.
Commit d85ce012f99 added this code but provided tests that did not
cover this correctly. This was fixed in the previous patch.
Reviewed-by: Tom Lane <[email protected]>
Discussion: https://www.postgresql.org/message-id/flat/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.org
---
src/backend/utils/adt/timestamp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 25cff56c3d0..e640b48205b 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -4954,7 +4954,7 @@ timestamptz_trunc_internal(text *units, TimestampTz timestamp, pg_tz *tzp)
case DTK_SECOND:
case DTK_MILLISEC:
case DTK_MICROSEC:
- PG_RETURN_TIMESTAMPTZ(timestamp);
+ return timestamp;
break;
default:
--
2.50.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: date_trunc invalid units with infinite value
@ 2025-08-07 00:06 Michael Paquier <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2025-08-07 00:06 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Joseph Koshakow <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Aug 06, 2025 at 01:07:25PM +0200, Peter Eisentraut wrote:
> It turned out this had a bug, and also the newly added test cases didn't
> actually cover the new code, otherwise this would have shown up.
>
> Please review the attached patches with additional test cases and the fix.
>
> See also [0] for further context:
>
> [0]: https://www.postgresql.org/message-id/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.org
Yes, confirmed the broken case on 32-bit builds with the incorrect
result returned by timestamptz_trunc_internal():
SELECT date_trunc( 'week', timestamp with time zone 'infinity' );
And confirmed that we don't have any coverage for two code paths as of
HEAD:
- "not supported" in timestamp_trunc() for the entire new section of
where TIMESTAMP_NOT_FINITE() is satisfied.
- "not supported" in timestamptz_trunc_internal() for the entire
section where TIMESTAMP_NOT_FINITE() is satisfied.
0001 adds three new tests for timestamp:
- TIMESTAMP_NOT_FINITE + a valid unit, new path.
- TIMESTAMP_NOT_FINITE + "not supported" unit, new error path
- !TIMESTAMP_NOT_FINITE + "not supported" unit, old error path
0001 four new tests for timestamptz:
1) Three tests for timestamptz_trunc():
- TIMESTAMP_NOT_FINITE + a valid unit, new path.
- TIMESTAMP_NOT_FINITE + "not supported" unit, new path.
- !TIMESTAMP_NOT_FINITE +
2) One test for timestamptz_trunc_zone():
- !TIMESTAMP_NOT_FINITE + "not supported" unit
With what I am reading in your patch, what you are suggesting to add,
and a double-check at the interval tests, that seems complete to me.
This is a v18 open item, for something that I am an owner of as the
committer of d85ce012f99f, so I'll go take care of it. Thanks!
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: date_trunc invalid units with infinite value
@ 2025-08-07 02:51 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2025-08-07 02:51 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Joseph Koshakow <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Aug 07, 2025 at 09:06:25AM +0900, Michael Paquier wrote:
> Yes, confirmed the broken case on 32-bit builds with the incorrect
> result returned by timestamptz_trunc_internal():
> SELECT date_trunc( 'week', timestamp with time zone 'infinity' );
>
> 0001 four new tests for timestamptz:
> 1) Three tests for timestamptz_trunc():
> - TIMESTAMP_NOT_FINITE + a valid unit, new path.
> - TIMESTAMP_NOT_FINITE + "not supported" unit, new path.
> - !TIMESTAMP_NOT_FINITE +
Blurp here. I meant !TIMESTAMP_NOT_FINITE with unsupported unit, old
code path.
> 2) One test for timestamptz_trunc_zone():
> - !TIMESTAMP_NOT_FINITE + "not supported" unit
There can be an extra test here, for the case of an infinite value
with a valid unit and a time zone specified, which would also have
failed with the bug as timestamptz_trunc_internal() is also used by
timestamptz_trunc_zone(), like:
SELECT date_trunc( 'week', timestamp with time zone 'infinity', 'GMT')
AS inf_zone_trunc;
I have added one more test, reversed the order to avoid spurious
failures should one have the idea to do a bisect with a 32b build, and
applied both things.
Thanks for the report!
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: date_trunc invalid units with infinite value
@ 2025-08-12 01:15 Joseph Koshakow <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Joseph Koshakow @ 2025-08-12 01:15 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; PostgreSQL Hackers <[email protected]>
Sorry about that and thanks for the fix!
- Joe Koshakow
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2025-08-12 01:15 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-04-02 00:56 [PATCH 3/8] Move processing of NULLs from BRIN support functions Tomas Vondra <[email protected]>
2024-12-05 05:34 Re: date_trunc invalid units with infinite value Michael Paquier <[email protected]>
2024-12-24 07:33 ` Re: date_trunc invalid units with infinite value Michael Paquier <[email protected]>
2024-12-27 04:42 ` Re: date_trunc invalid units with infinite value Michael Paquier <[email protected]>
2025-08-06 11:07 ` Re: date_trunc invalid units with infinite value Peter Eisentraut <[email protected]>
2025-08-07 00:06 ` Re: date_trunc invalid units with infinite value Michael Paquier <[email protected]>
2025-08-07 02:51 ` Re: date_trunc invalid units with infinite value Michael Paquier <[email protected]>
2025-08-12 01:15 ` Re: date_trunc invalid units with infinite value Joseph Koshakow <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox