public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v34 6/8] Allow CLUSTER and VACUUM FULL to change tablespace
10+ messages / 3 participants
[nested] [flat]
* [PATCH v34 6/8] Allow CLUSTER and VACUUM FULL to change tablespace
@ 2020-03-24 15:16 Alexey Kondratov <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Alexey Kondratov @ 2020-03-24 15:16 UTC (permalink / raw)
---
doc/src/sgml/ref/cluster.sgml | 19 ++++---
doc/src/sgml/ref/vacuum.sgml | 20 +++++++
src/backend/commands/cluster.c | 67 +++++++++++++++++++++--
src/backend/commands/tablecmds.c | 5 +-
src/backend/commands/vacuum.c | 55 ++++++++++++++++++-
src/backend/parser/gram.y | 5 +-
src/backend/postmaster/autovacuum.c | 1 +
src/bin/psql/tab-complete.c | 9 ++-
src/include/commands/cluster.h | 3 +-
src/include/commands/vacuum.h | 2 +
src/test/regress/input/tablespace.source | 23 +++++++-
src/test/regress/output/tablespace.source | 37 ++++++++++++-
12 files changed, 221 insertions(+), 25 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 5dd21a0189..6758ef97a6 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -28,6 +28,7 @@ CLUSTER [VERBOSE]
<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
+ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
</synopsis>
</refsynopsisdiv>
@@ -105,6 +106,15 @@ CLUSTER [VERBOSE]
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the table will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>VERBOSE</literal></term>
<listitem>
@@ -115,15 +125,10 @@ CLUSTER [VERBOSE]
</varlistentry>
<varlistentry>
- <term><replaceable class="parameter">boolean</replaceable></term>
+ <term><replaceable class="parameter">new_tablespace</replaceable></term>
<listitem>
<para>
- Specifies whether the selected option should be turned on or off.
- You can write <literal>TRUE</literal>, <literal>ON</literal>, or
- <literal>1</literal> to enable the option, and <literal>FALSE</literal>,
- <literal>OFF</literal>, or <literal>0</literal> to disable it. The
- <replaceable class="parameter">boolean</replaceable> value can also
- be omitted, in which case <literal>TRUE</literal> is assumed.
+ The tablespace where the table will be rebuilt.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 21ab57d880..5261a7c727 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -35,6 +35,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
INDEX_CLEANUP [ <replaceable class="parameter">boolean</replaceable> ]
TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
PARALLEL <replaceable class="parameter">integer</replaceable>
+ TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -255,6 +256,15 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the relation will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
@@ -299,6 +309,16 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="parameter">new_tablespace</replaceable></term>
+ <listitem>
+ <para>
+ The tablespace where the relation will be rebuilt.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect1>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 272723e050..39b32bb85f 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -33,11 +33,13 @@
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_tablespace.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
#include "commands/defrem.h"
#include "commands/progress.h"
#include "commands/tablecmds.h"
+#include "commands/tablespace.h"
#include "commands/vacuum.h"
#include "miscadmin.h"
#include "optimizer/optimizer.h"
@@ -68,7 +70,8 @@ typedef struct
} RelToCluster;
-static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose);
+static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose,
+ Oid NewTableSpaceOid);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
@@ -105,6 +108,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
ListCell *lc;
int options = 0;
bool verbose = false;
+ /* Name and Oid of tablespace to use for clustered relation. */
+ char *tablespaceName = NULL;
+ Oid tablespaceOid = InvalidOid;
/* Parse option list */
foreach(lc, stmt->params)
@@ -113,6 +119,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
if (strcmp(opt->defname, "verbose") == 0)
verbose = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "tablespace") == 0)
+ tablespaceName = defGetString(opt);
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -123,6 +131,19 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
options = (verbose ? CLUOPT_VERBOSE : 0);
+ /* Select tablespace Oid to use. */
+ if (tablespaceName)
+ {
+ tablespaceOid = get_tablespace_oid(tablespaceName, false);
+
+ /* Can't move a non-shared relation into pg_global */
+ if (tablespaceOid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ tablespaceName)));
+ }
+
if (stmt->relation != NULL)
{
/* This is the single-relation case. */
@@ -192,7 +213,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
table_close(rel, NoLock);
/* Do the job. */
- cluster_rel(tableOid, indexOid, options);
+ cluster_rel(tableOid, indexOid, options,
+ tablespaceOid);
}
else
{
@@ -241,7 +263,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
PushActiveSnapshot(GetTransactionSnapshot());
/* Do the job. */
cluster_rel(rvtc->tableOid, rvtc->indexOid,
- options | CLUOPT_RECHECK);
+ options | CLUOPT_RECHECK,
+ tablespaceOid);
PopActiveSnapshot();
CommitTransactionCommand();
}
@@ -270,9 +293,12 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
* If indexOid is InvalidOid, the table will be rewritten in physical order
* instead of index order. This is the new implementation of VACUUM FULL,
* and error messages should refer to the operation as VACUUM not CLUSTER.
+ *
+ * "tablespaceOid" is the tablespace where the relation will be rebuilt, or
+ * InvalidOid to use its current tablespace.
*/
void
-cluster_rel(Oid tableOid, Oid indexOid, int options)
+cluster_rel(Oid tableOid, Oid indexOid, int options, Oid tablespaceOid)
{
Relation OldHeap;
bool verbose = ((options & CLUOPT_VERBOSE) != 0);
@@ -372,6 +398,23 @@ cluster_rel(Oid tableOid, Oid indexOid, int options)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot cluster a shared catalog")));
+ if (OidIsValid(tablespaceOid) &&
+ !allowSystemTableMods && IsSystemRelation(OldHeap))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system catalog",
+ RelationGetRelationName(OldHeap))));
+
+ /*
+ * We cannot support moving mapped relations into different tablespaces.
+ * (In particular this eliminates all shared catalogs.)
+ */
+ if (OidIsValid(tablespaceOid) && RelationIsMapped(OldHeap))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot change tablespace of mapped relation \"%s\"",
+ RelationGetRelationName(OldHeap))));
+
/*
* Don't process temp tables of other backends ... their local buffer
* manager is not going to cope.
@@ -422,7 +465,8 @@ cluster_rel(Oid tableOid, Oid indexOid, int options)
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- rebuild_relation(OldHeap, indexOid, verbose);
+ rebuild_relation(OldHeap, indexOid, verbose,
+ tablespaceOid);
/* NB: rebuild_relation does table_close() on OldHeap */
@@ -571,7 +615,7 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
* NB: this routine closes OldHeap at the right time; caller should not.
*/
static void
-rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
+rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
@@ -582,6 +626,10 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
TransactionId frozenXid;
MultiXactId cutoffMulti;
+ /* Use new tablespace if passed. */
+ if (OidIsValid(NewTablespaceOid))
+ tableSpace = NewTablespaceOid;
+
/* Mark the correct index as clustered */
if (OidIsValid(indexOid))
mark_index_clustered(OldHeap, indexOid, true);
@@ -1026,6 +1074,13 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
*/
Assert(!target_is_pg_class);
+ if (!allowSystemTableMods && IsSystemClass(r1, relform1) &&
+ relform1->reltablespace != relform2->reltablespace)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system catalog",
+ get_rel_name(r1))));
+
swaptemp = relform1->relfilenode;
relform1->relfilenode = relform2->relfilenode;
relform2->relfilenode = swaptemp;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb96e330d4..3fb726db63 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13196,8 +13196,9 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
if (RelationIsMapped(rel))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move system relation \"%s\"",
- RelationGetRelationName(rel))));
+ errmsg("cannot change tablespace of mapped relation \"%s\"",
+ RelationGetRelationName(rel))));
+
/* Can't move a non-shared relation into pg_global */
if (newTableSpace == GLOBALTABLESPACE_OID)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 98270a1049..6861e948f2 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -31,13 +31,16 @@
#include "access/tableam.h"
#include "access/transam.h"
#include "access/xact.h"
+#include "catalog/catalog.h"
#include "catalog/namespace.h"
#include "catalog/pg_database.h"
#include "catalog/pg_inherits.h"
#include "catalog/pg_namespace.h"
+#include "catalog/pg_tablespace.h"
#include "commands/cluster.h"
#include "commands/defrem.h"
#include "commands/vacuum.h"
+#include "commands/tablespace.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "pgstat.h"
@@ -106,6 +109,10 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
bool disable_page_skipping = false;
ListCell *lc;
+ /* Name and Oid of tablespace to use for relations after VACUUM FULL. */
+ char *tablespacename = NULL;
+ Oid tablespaceOid = InvalidOid;
+
/* Set default value */
params.index_cleanup = VACOPT_TERNARY_DEFAULT;
params.truncate = VACOPT_TERNARY_DEFAULT;
@@ -142,6 +149,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.index_cleanup = get_vacopt_ternary_value(opt);
else if (strcmp(opt->defname, "truncate") == 0)
params.truncate = get_vacopt_ternary_value(opt);
+ else if (strcmp(opt->defname, "tablespace") == 0)
+ tablespacename = defGetString(opt);
else if (strcmp(opt->defname, "parallel") == 0)
{
if (opt->arg == NULL)
@@ -202,6 +211,27 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("VACUUM FULL cannot be performed in parallel")));
+ /* Get tablespace Oid to use. */
+ if (tablespacename)
+ {
+ if ((params.options & VACOPT_FULL) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("incompatible TABLESPACE option"),
+ errdetail("TABLESPACE can only be used with VACUUM FULL.")));
+
+ tablespaceOid = get_tablespace_oid(tablespacename, false);
+
+ /* Can't move a non-shared relation into pg_global */
+ if (tablespaceOid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ tablespacename)));
+
+ }
+ params.tablespace_oid = tablespaceOid;
+
/*
* Make sure VACOPT_ANALYZE is specified if any column lists are present.
*/
@@ -1718,8 +1748,9 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
LOCKMODE lmode;
Relation onerel;
LockRelId onerelid;
- Oid toast_relid;
- Oid save_userid;
+ Oid toast_relid,
+ save_userid,
+ tablespaceOid = InvalidOid;
int save_sec_context;
int save_nestlevel;
@@ -1857,6 +1888,23 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
return true;
}
+ /*
+ * We don't support moving system relations into different tablespaces
+ * unless allow_system_table_mods=1.
+ */
+ if (params->options & VACOPT_FULL &&
+ OidIsValid(params->tablespace_oid) &&
+ IsSystemRelation(onerel) && !allowSystemTableMods)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("skipping tablespace change of \"%s\"",
+ RelationGetRelationName(onerel)),
+ errdetail("Cannot move system relation, only VACUUM is performed.")));
+ }
+ else
+ tablespaceOid = params->tablespace_oid;
+
/*
* Get a session-level lock too. This will protect our access to the
* relation across multiple transactions, so that we can vacuum the
@@ -1926,7 +1974,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
cluster_options |= CLUOPT_VERBOSE;
/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
- cluster_rel(relid, InvalidOid, cluster_options);
+ cluster_rel(relid, InvalidOid, cluster_options,
+ tablespaceOid);
}
else
table_relation_vacuum(onerel, params, vac_strategy);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 8f341ac006..2ef9485ab2 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10444,8 +10444,9 @@ cluster_index_specification:
/*****************************************************************************
*
* QUERY:
- * VACUUM
- * ANALYZE
+ * VACUUM [FULL] [FREEZE] [VERBOSE] [ANALYZE] [ <table_and_columns> [, ...] ]
+ * VACUUM [(options)] [ <table_and_columns> [, ...] ]
+ * ANALYZE [VERBOSE] [ <table_and_columns> [, ...] ]
*
*****************************************************************************/
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 7e28944d2f..d2a443ebdb 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2933,6 +2933,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age;
tab->at_params.is_wraparound = wraparound;
tab->at_params.log_min_duration = log_min_duration;
+ tab->at_params.tablespace_oid = InvalidOid;
tab->at_vacuum_cost_limit = vac_cost_limit;
tab->at_vacuum_cost_delay = vac_cost_delay;
tab->at_relname = NULL;
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 65ebf911f3..0908d7d4c7 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2312,7 +2312,9 @@ psql_completion(const char *text, int start, int end)
* one word, so the above test is correct.
*/
if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
- COMPLETE_WITH("VERBOSE");
+ COMPLETE_WITH("TABLESPACE", "VERBOSE");
+ else if (TailMatches("TABLESPACE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
}
/* COMMENT */
@@ -3806,9 +3808,12 @@ psql_completion(const char *text, int start, int end)
if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
COMPLETE_WITH("FULL", "FREEZE", "ANALYZE", "VERBOSE",
"DISABLE_PAGE_SKIPPING", "SKIP_LOCKED",
- "INDEX_CLEANUP", "TRUNCATE", "PARALLEL");
+ "INDEX_CLEANUP", "TRUNCATE", "PARALLEL",
+ "TABLESPACE");
else if (TailMatches("FULL|FREEZE|ANALYZE|VERBOSE|DISABLE_PAGE_SKIPPING|SKIP_LOCKED|INDEX_CLEANUP|TRUNCATE"))
COMPLETE_WITH("ON", "OFF");
+ else if (TailMatches("TABLESPACE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
}
else if (HeadMatches("VACUUM") && TailMatches("("))
/* "VACUUM (" should be caught above, so assume we want columns */
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 7cfb37c9b2..30ef24c41b 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -27,7 +27,8 @@ typedef enum ClusterOption
} ClusterOption;
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
-extern void cluster_rel(Oid tableOid, Oid indexOid, int options);
+extern void cluster_rel(Oid tableOid, Oid indexOid, int options,
+ Oid tablespaceOid);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index a4cd721400..4b5ac7145d 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -229,6 +229,8 @@ typedef struct VacuumParams
* disabled.
*/
int nworkers;
+ Oid tablespace_oid; /* tablespace to use for relations
+ * rebuilt by VACUUM FULL */
} VacuumParams;
/* GUC parameters */
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 5d8a22cffb..f4687f6bfb 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -17,7 +17,7 @@ ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- f
ALTER TABLESPACE regress_tblspace RESET (random_page_cost = 2.0); -- fail
ALTER TABLESPACE regress_tblspace RESET (random_page_cost, effective_io_concurrency); -- ok
--- create table to test REINDEX with TABLESPACE change
+-- create table to test REINDEX, CLUSTER and VACUUM FULL with TABLESPACE change
CREATE TABLE regress_tblspace_test_tbl (num1 bigint, num2 double precision, num3 double precision);
INSERT INTO regress_tblspace_test_tbl (num1, num2, num3)
SELECT round(random()*100), random(), random()*42
@@ -47,11 +47,32 @@ REINDEX (TABLESPACE regress_tblspace) TABLE CONCURRENTLY pg_am; -- fail
REINDEX (TABLESPACE pg_global) INDEX regress_tblspace_test_tbl_idx; -- fail
REINDEX (TABLESPACE regress_tblspace) TABLE pg_am; -- fail
+-- check that all indexes moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+
+-- check CLUSTER with TABLESPACE change
+CLUSTER (TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+CLUSTER (TABLESPACE regress_tblspace) pg_authid USING pg_authid_rolname_index; -- fail
+CLUSTER (TABLESPACE pg_global) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- fail
+
-- check that all relations moved to new tablespace
SELECT relname FROM pg_class
WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
ORDER BY relname;
+-- check VACUUM with TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+VACUUM (FULL, TABLESPACE pg_default) pg_authid; -- skip with warning
+VACUUM (ANALYSE, TABLESPACE pg_default); -- fail
+VACUUM (FULL, TABLESPACE pg_global) regress_tblspace_test_tbl; -- fail
+
+-- check that all tables moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+
-- move indexes back to pg_default tablespace
REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; -- ok
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 1169f0318b..19c9504c05 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -20,7 +20,7 @@ ERROR: unrecognized parameter "some_nonexistent_parameter"
ALTER TABLESPACE regress_tblspace RESET (random_page_cost = 2.0); -- fail
ERROR: RESET must not include values for parameters
ALTER TABLESPACE regress_tblspace RESET (random_page_cost, effective_io_concurrency); -- ok
--- create table to test REINDEX with TABLESPACE change
+-- create table to test REINDEX, CLUSTER and VACUUM FULL with TABLESPACE change
CREATE TABLE regress_tblspace_test_tbl (num1 bigint, num2 double precision, num3 double precision);
INSERT INTO regress_tblspace_test_tbl (num1, num2, num3)
SELECT round(random()*100), random(), random()*42
@@ -61,9 +61,44 @@ REINDEX (TABLESPACE pg_global) INDEX regress_tblspace_test_tbl_idx; -- fail
ERROR: cannot move non-shared relation to tablespace "pg_global"
REINDEX (TABLESPACE regress_tblspace) TABLE pg_am; -- fail
ERROR: permission denied: "pg_am_name_index" is a system catalog
+-- check that all indexes moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+ relname
+-------------------------------
+ regress_tblspace_test_tbl_idx
+(1 row)
+
+-- check CLUSTER with TABLESPACE change
+CLUSTER (TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+CLUSTER (TABLESPACE regress_tblspace) pg_authid USING pg_authid_rolname_index; -- fail
+ERROR: cannot cluster a shared catalog
+CLUSTER (TABLESPACE pg_global) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- fail
+ERROR: cannot move non-shared relation to tablespace "pg_global"
-- check that all relations moved to new tablespace
SELECT relname FROM pg_class
WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+ relname
+-------------------------------
+ regress_tblspace_test_tbl
+ regress_tblspace_test_tbl_idx
+(2 rows)
+
+-- check VACUUM with TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+VACUUM (FULL, TABLESPACE pg_default) pg_authid; -- skip with warning
+WARNING: skipping tablespace change of "pg_authid"
+DETAIL: Cannot move system relation, only VACUUM is performed.
+VACUUM (ANALYSE, TABLESPACE pg_default); -- fail
+ERROR: incompatible TABLESPACE option
+DETAIL: TABLESPACE can only be used with VACUUM FULL.
+VACUUM (FULL, TABLESPACE pg_global) regress_tblspace_test_tbl; -- fail
+ERROR: cannot move non-shared relation to tablespace "pg_global"
+-- check that all tables moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
ORDER BY relname;
relname
-------------------------------
--
2.17.0
--ucW7QTJbHsz5ya91
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v34-0007-Implement-vacuum-full-cluster-INDEX_TABLESPACE-t.patch"
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-09 14:07 David E. Wheeler <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: David E. Wheeler @ 2024-07-09 14:07 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> On Jul 8, 2024, at 21:44, Junwang Zhao <[email protected]> wrote:
>
> # select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
>
> Do you also expect this to show the time in America/New_York?
>
> This is what I get:
>
> [local] postgres@postgres:5432-28176=# select
> jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
> ┌─────────────────────────────┐
> │ jsonb_path_query_tz │
> ├─────────────────────────────┤
> │ "2024-08-15T12:34:56-05:00" │
> └─────────────────────────────┘
> (1 row)
>
> The logic in executeDateTimeMethod seems to convert the input to a UTC
> timestamp base on the session TZ,
> the output seems not cast based on the TZ.
Right, which now that I think about it seems odd, because timestamptz does not actually store an offset. As you say, it converts the time to UTC and stores only that, then displays the offset relative to the current time zone setting.
So in plain SQL it always displays relative to the current TZ offset:
david=# set time zone 'America/New_York';
SET
david=# select '2023-08-15 12:34:56-05'::timestamptz;
timestamptz
------------------------
2023-08-15 13:34:56-04
(1 row)
david=# select '2023-08-15 12:34:56'::timestamptz;
timestamptz
------------------------
2023-08-15 12:34:56-04
(1 row)
In jsopath expressions, however, it does not, as your example demonstrates:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T12:34:56-05:00"
How is it retaining the offset? Should it?
The display is properly adjusted when using string():
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz().string()');
jsonb_path_query_tz
--------------------------
"2024-08-15 13:34:56-04"
(1 row)
So perhaps I had things reversed before. Maybe it’s actually doing the right then when it converts a timestamp to a timestamptz, but not when it the input contains an offset, as in your example.
D
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-09 14:22 David E. Wheeler <[email protected]>
parent: David E. Wheeler <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: David E. Wheeler @ 2024-07-09 14:22 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Jul 9, 2024, at 10:07, David E. Wheeler <[email protected]> wrote:
> So perhaps I had things reversed before. Maybe it’s actually doing the right then when it converts a timestamp to a timestamptz, but not when it the input contains an offset, as in your example.
To clarify, there’s an inconsistency in the output of timestamp_tz() depending on whether the input has an offset or not. With offset:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T12:34:56-05:00"
And without:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T16:34:56+00:00"
I suspect the latter is correct, given that the timestamptz type appears to be an int64, presumably always in UTC. I don’t understand where the first example stores the offset.
Best,
David
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-09 15:08 Junwang Zhao <[email protected]>
parent: David E. Wheeler <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Junwang Zhao @ 2024-07-09 15:08 UTC (permalink / raw)
To: David E. Wheeler <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Tue, Jul 9, 2024 at 10:22 PM David E. Wheeler <[email protected]> wrote:
>
> On Jul 9, 2024, at 10:07, David E. Wheeler <[email protected]> wrote:
>
> > So perhaps I had things reversed before. Maybe it’s actually doing the right then when it converts a timestamp to a timestamptz, but not when it the input contains an offset, as in your example.
>
> To clarify, there’s an inconsistency in the output of timestamp_tz() depending on whether the input has an offset or not. With offset:
>
> david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
> jsonb_path_query_tz
> -----------------------------
> "2024-08-15T12:34:56-05:00"
>
> And without:
>
> david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
> jsonb_path_query_tz
> -----------------------------
> "2024-08-15T16:34:56+00:00"
>
> I suspect the latter is correct, given that the timestamptz type appears to be an int64, presumably always in UTC. I don’t understand where the first example stores the offset.
In JsonbValue.val.datatime, there is a tz field, I think that's where
the offset stored, it is 18000 in the first example
struct
{
Datum value;
Oid typid;
int32 typmod;
int tz; /* Numeric time zone, in seconds, for
* TimestampTz data type */
} datetime;
>
> Best,
>
> David
>
>
>
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-09 15:38 David E. Wheeler <[email protected]>
parent: Junwang Zhao <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: David E. Wheeler @ 2024-07-09 15:38 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Jul 9, 2024, at 11:08, Junwang Zhao <[email protected]> wrote:
> In JsonbValue.val.datatime, there is a tz field, I think that's where
> the offset stored, it is 18000 in the first example
>
> struct
> {
> Datum value;
> Oid typid;
> int32 typmod;
> int tz; /* Numeric time zone, in seconds, for
> * TimestampTz data type */
> } datetime;
Oooh, okay, so it’s a jsonb variant of the type. Interesting. Ah, and it’s assigned here[1]:
jb->val.datetime.tz = tz;
It seems like JSONB timestamptz values want to display the recorded time zone, so I suspect we need to set it when the converting from a non-tz to a local tz setting, something like this:
``` patch
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index d79c929822..f63b3b9330 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2707,12 +2707,16 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
break;
case jpiTimestampTz:
{
+ struct pg_tm *tm;
/* Convert result type to timestamp with time zone */
switch (typid)
{
case DATEOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"date", "timestamptz");
+ if (timestamp2tm(DatumGetTimestamp(value), NULL, tm, NULL, NULL, NULL) == 0) {
+ tz = DetermineTimeZoneOffset(tm, session_timezone);
+ }
value = DirectFunctionCall1(date_timestamptz,
value);
break;
@@ -2726,6 +2730,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
case TIMESTAMPOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"timestamp", "timestamptz");
+ if (timestamp2tm(DatumGetTimestamp(value), NULL, tm, NULL, NULL, NULL) == 0) {
+ tz = DetermineTimeZoneOffset(tm, session_timezone);
+ }
value = DirectFunctionCall1(timestamp_timestamptz,
value);
break;
```
Only, you know, doesn’t crash the server.
Best,
David
[1]: https://github.com/postgres/postgres/blob/629520be5f9da9d0192c7f6c8796bfddb4746760/src/backend/utils...
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-10 05:48 Junwang Zhao <[email protected]>
parent: David E. Wheeler <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Junwang Zhao @ 2024-07-10 05:48 UTC (permalink / raw)
To: David E. Wheeler <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Tue, Jul 9, 2024 at 11:38 PM David E. Wheeler <[email protected]> wrote:
>
> On Jul 9, 2024, at 11:08, Junwang Zhao <[email protected]> wrote:
>
> > In JsonbValue.val.datatime, there is a tz field, I think that's where
> > the offset stored, it is 18000 in the first example
> >
> > struct
> > {
> > Datum value;
> > Oid typid;
> > int32 typmod;
> > int tz; /* Numeric time zone, in seconds, for
> > * TimestampTz data type */
> > } datetime;
>
> Oooh, okay, so it’s a jsonb variant of the type. Interesting. Ah, and it’s assigned here[1]:
>
> jb->val.datetime.tz = tz;
>
> It seems like JSONB timestamptz values want to display the recorded time zone, so I suspect we need to set it when the converting from a non-tz to a local tz setting, something like this:
>
> ``` patch
> diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
> index d79c929822..f63b3b9330 100644
> --- a/src/backend/utils/adt/jsonpath_exec.c
> +++ b/src/backend/utils/adt/jsonpath_exec.c
> @@ -2707,12 +2707,16 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
> break;
> case jpiTimestampTz:
> {
> + struct pg_tm *tm;
> /* Convert result type to timestamp with time zone */
> switch (typid)
> {
> case DATEOID:
> checkTimezoneIsUsedForCast(cxt->useTz,
> "date", "timestamptz");
> + if (timestamp2tm(DatumGetTimestamp(value), NULL, tm, NULL, NULL, NULL) == 0) {
> + tz = DetermineTimeZoneOffset(tm, session_timezone);
> + }
> value = DirectFunctionCall1(date_timestamptz,
> value);
> break;
> @@ -2726,6 +2730,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
> case TIMESTAMPOID:
> checkTimezoneIsUsedForCast(cxt->useTz,
> "timestamp", "timestamptz");
> + if (timestamp2tm(DatumGetTimestamp(value), NULL, tm, NULL, NULL, NULL) == 0) {
> + tz = DetermineTimeZoneOffset(tm, session_timezone);
> + }
> value = DirectFunctionCall1(timestamp_timestamptz,
> value);
> break;
> ```
>
> Only, you know, doesn’t crash the server.
I apply your patch with some minor change(to make the server not crash):
diff --git a/src/backend/utils/adt/jsonpath_exec.c
b/src/backend/utils/adt/jsonpath_exec.c
index d79c9298227..87a695ef633 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2708,6 +2708,8 @@ executeDateTimeMethod(JsonPathExecContext *cxt,
JsonPathItem *jsp,
case jpiTimestampTz:
{
/* Convert result type to timestamp
with time zone */
+ struct pg_tm tm;
+ fsec_t fsec;
switch (typid)
{
case DATEOID:
@@ -2726,6 +2728,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt,
JsonPathItem *jsp,
case TIMESTAMPOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"timestamp", "timestamptz");
+ if
(timestamp2tm(DatumGetTimestamp(value), NULL, &tm, &fsec, NULL, NULL)
== 0) {
+ tz =
DetermineTimeZoneOffset(&tm, session_timezone);
+ }
value =
DirectFunctionCall1(timestamp_timestamptz,
value);
break;
It now gives the local tz:
[local] postgres@postgres:5432-54960=# set time zone 'America/New_York';
SET
Time: 2.894 ms
[local] postgres@postgres:5432-54960=# select
jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
┌─────────────────────────────┐
│ jsonb_path_query_tz │
├─────────────────────────────┤
│ "2024-08-15T12:34:56-04:00" │
└─────────────────────────────┘
(1 row)
Time: 293813.022 ms (04:53.813)
I'm not sure whether the SQL/JSON standard mentioned this, I searched a
little bit, but found no clue :(
>
> Best,
>
> David
>
>
> [1]: https://github.com/postgres/postgres/blob/629520be5f9da9d0192c7f6c8796bfddb4746760/src/backend/utils...
>
>
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-10 14:33 David E. Wheeler <[email protected]>
parent: Junwang Zhao <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: David E. Wheeler @ 2024-07-10 14:33 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Jul 10, 2024, at 01:48, Junwang Zhao <[email protected]> wrote:
> I apply your patch with some minor change(to make the server not crash):
Oh, thank you! Kicking myself for not catching the obvious.
> It now gives the local tz:
>
> [local] postgres@postgres:5432-54960=# set time zone 'America/New_York';
> SET
> Time: 2.894 ms
> [local] postgres@postgres:5432-54960=# select
> jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
> ┌─────────────────────────────┐
> │ jsonb_path_query_tz │
> ├─────────────────────────────┤
> │ "2024-08-15T12:34:56-04:00" │
> └─────────────────────────────┘
> (1 row)
>
> Time: 293813.022 ms (04:53.813)
Yes, and I think that’s what we want, since it preserves and displays the offset for strings that contain them:
david=# set time zone 'America/New_York';
SET
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56+10"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T12:34:56+10:00"
> I'm not sure whether the SQL/JSON standard mentioned this, I searched a
> little bit, but found no clue :(
Yeah I don’t know either, but now at least it’s consistent. I’ve attached a patch to fix it.
Ideally, I think, we wouldn’t convert the value and determine the offset twice, but teach date_timestamptz and timestamp_timestamptz (or date2timestamptz and timestamp2timestamptz?) how to return the offset, or create alternate functions that do so. Not sure what calling style should be adopted here, but this at least addresses the issue. Happy to resubmit something more efficient upon function design feedback.
Best,
David
Attachments:
[application/octet-stream] v1-0001-Preserve-tz-when-converting-to-jsonb-timestamptz.patch (4.0K, ../../[email protected]/2-v1-0001-Preserve-tz-when-converting-to-jsonb-timestamptz.patch)
download | inline diff:
From 31fea30552dc4e6a05c969adf61cdb010cfb9fbe Mon Sep 17 00:00:00 2001
From: "David E. Wheeler" <[email protected]>
Date: Wed, 10 Jul 2024 10:23:24 -0400
Subject: [PATCH v1] Preserve tz when converting to jsonb timestamptz
The JSONB jbvDatetime type has a field for offset, and displays the time
in that offset. For example, when the time zone GUC is set to
America/New_York, the jsonpath `timestamp_tz()` method returns a value
that displays a parsed value with its offset, not the local offset:
david=# set time zone 'America/New_York';
SET
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56+10"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T12:34:56+10:00"
This was not true for values parsed by `timestamp_tz()` that lacked an
offset. It correctly assumes the local time zone, but displays it in
UTC:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T16:34:56+00:00"
To fix this inconsistent behavior, determine the offset for values being
cast from `DATEOID` and `DATEOID` types to `jpiTimestampTz` and store it
in the resulting jbvDatetime value. With this change, the result now
preserves the offset just as it does when converting from offset-aware
values:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2023-08-15T12:34:56-04:00"
Author: David Wheeler
Reviewed-by: Junwang Zhao
Discussion: https://postgr.es/m/7DE080CE-6D8C-4794-9BD1-7D9699172FAB%40justatheory.com
---
src/backend/utils/adt/jsonpath_exec.c | 8 ++++++++
src/test/regress/expected/jsonb_jsonpath.out | 4 ++--
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index d79c929822..f7d255746b 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2707,12 +2707,17 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
break;
case jpiTimestampTz:
{
+ struct pg_tm tm;
+ fsec_t fsec;
/* Convert result type to timestamp with time zone */
switch (typid)
{
case DATEOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"date", "timestamptz");
+ if (timestamp2tm(DatumGetTimestamp(value), NULL, &tm, &fsec, NULL, NULL) == 0) {
+ tz = DetermineTimeZoneOffset(&tm, session_timezone);
+ }
value = DirectFunctionCall1(date_timestamptz,
value);
break;
@@ -2726,6 +2731,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
case TIMESTAMPOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"timestamp", "timestamptz");
+ if (timestamp2tm(DatumGetTimestamp(value), NULL, &tm, &fsec, NULL, NULL) == 0) {
+ tz = DetermineTimeZoneOffset(&tm, session_timezone);
+ }
value = DirectFunctionCall1(timestamp_timestamptz,
value);
break;
diff --git a/src/test/regress/expected/jsonb_jsonpath.out b/src/test/regress/expected/jsonb_jsonpath.out
index a6112e86fa..62cc25f159 100644
--- a/src/test/regress/expected/jsonb_jsonpath.out
+++ b/src/test/regress/expected/jsonb_jsonpath.out
@@ -2914,7 +2914,7 @@ HINT: Use *_tz() function for time zone support.
select jsonb_path_query_tz('"2023-08-15"', '$.timestamp_tz()'); -- should work
jsonb_path_query_tz
-----------------------------
- "2023-08-15T07:00:00+00:00"
+ "2023-08-14T23:00:00-08:00"
(1 row)
select jsonb_path_query('"12:34:56"', '$.timestamp_tz()');
@@ -3101,7 +3101,7 @@ HINT: Use *_tz() function for time zone support.
select jsonb_path_query_tz('"2023-08-15 12:34:56"', '$.timestamp_tz()'); -- should work
jsonb_path_query_tz
-----------------------------
- "2023-08-15T02:34:56+00:00"
+ "2023-08-15T12:34:56+10:00"
(1 row)
select jsonb_path_query('"2023-08-15 12:34:56 +05:30"', '$.timestamp_tz()');
--
2.45.2
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-10 14:54 David E. Wheeler <[email protected]>
parent: David E. Wheeler <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: David E. Wheeler @ 2024-07-10 14:54 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Jul 10, 2024, at 10:33, David E. Wheeler <[email protected]> wrote:
> Yeah I don’t know either, but now at least it’s consistent. I’ve attached a patch to fix it.
Actually I think there’s a subtlety still missing here:
@@ -2914,7 +2914,7 @@ HINT: Use *_tz() function for time zone support.
select jsonb_path_query_tz('"2023-08-15"', '$.timestamp_tz()'); -- should work
jsonb_path_query_tz -----------------------------
- "2023-08-15T07:00:00+00:00"
+ "2023-08-14T23:00:00-08:00"
(1 row)
This test runs while the time zone is set to “PST8PDT”, but it’s got the PST offset when it should be PDT:
david=# set time zone 'PST8PDT';
SET
david=# select '2023-08-15'::timestamptz;
timestamptz
------------------------
2023-08-15 00:00:00-07
So it should be -7, not -8. Not sure where to tell it to pay proper attention to daylight savings time.
Best,
David
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-10 15:19 David E. Wheeler <[email protected]>
parent: David E. Wheeler <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: David E. Wheeler @ 2024-07-10 15:19 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Jul 10, 2024, at 10:54, David E. Wheeler <[email protected]> wrote:
> So it should be -7, not -8. Not sure where to tell it to pay proper attention to daylight savings time.
Oh, and the time and date were wrong, too, because I blindly used the same conversion for dates as for timestamps. Fixed in v2.
PR: https://github.com/theory/postgres/pull/7
CF: https://commitfest.postgresql.org/49/5119/
Best,
David
Attachments:
[application/octet-stream] v2-0001-Preserve-tz-when-converting-to-jsonb-timestamptz.patch (4.1K, ../../[email protected]/2-v2-0001-Preserve-tz-when-converting-to-jsonb-timestamptz.patch)
download | inline diff:
From 6188d9b42df1d12c728cb09cbcda206475d36732 Mon Sep 17 00:00:00 2001
From: "David E. Wheeler" <[email protected]>
Date: Wed, 10 Jul 2024 11:17:54 -0400
Subject: [PATCH v2] Preserve tz when converting to jsonb timestamptz
The JSONB jbvDatetime type has a field for offset, and displays the time
in that offset. For example, when the time zone GUC is set to
America/New_York, the jsonpath `timestamp_tz()` method returns a value
that displays a parsed value with its offset, not the local offset:
david=# set time zone 'America/New_York';
SET
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56+10"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T12:34:56+10:00"
This was not true for values parsed by `timestamp_tz()` that lacked an
offset. It correctly assumes the local time zone, but displays it in
UTC:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T16:34:56+00:00"
To fix this inconsistent behavior, determine the offset for values being
cast from `DATEOID` and `DATEOID` types to `jpiTimestampTz` and store it
in the resulting jbvDatetime value. With this change, the result now
preserves the offset just as it does when converting from offset-aware
values:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2023-08-15T12:34:56-04:00"
Author: David Wheeler
Reviewed-by: Junwang Zhao
Discussion: https://postgr.es/m/7DE080CE-6D8C-4794-9BD1-7D9699172FAB%40justatheory.com
---
src/backend/utils/adt/jsonpath_exec.c | 12 ++++++++++++
src/test/regress/expected/jsonb_jsonpath.out | 4 ++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index d79c929822..ac174bbaa6 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2707,12 +2707,21 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
break;
case jpiTimestampTz:
{
+ struct pg_tm tm;
+ fsec_t fsec;
/* Convert result type to timestamp with time zone */
switch (typid)
{
case DATEOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"date", "timestamptz");
+ DateADT dateVal = DatumGetDateADT(value);
+ j2date(dateVal + POSTGRES_EPOCH_JDATE,
+ &(tm.tm_year), &(tm.tm_mon), &(tm.tm_mday));
+ tm.tm_hour = 0;
+ tm.tm_min = 0;
+ tm.tm_sec = 0;
+ tz = DetermineTimeZoneOffset(&tm, session_timezone);
value = DirectFunctionCall1(date_timestamptz,
value);
break;
@@ -2726,6 +2735,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
case TIMESTAMPOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"timestamp", "timestamptz");
+ if (timestamp2tm(DatumGetTimestamp(value), NULL, &tm, &fsec, NULL, NULL) == 0) {
+ tz = DetermineTimeZoneOffset(&tm, session_timezone);
+ }
value = DirectFunctionCall1(timestamp_timestamptz,
value);
break;
diff --git a/src/test/regress/expected/jsonb_jsonpath.out b/src/test/regress/expected/jsonb_jsonpath.out
index a6112e86fa..90e705ff14 100644
--- a/src/test/regress/expected/jsonb_jsonpath.out
+++ b/src/test/regress/expected/jsonb_jsonpath.out
@@ -2914,7 +2914,7 @@ HINT: Use *_tz() function for time zone support.
select jsonb_path_query_tz('"2023-08-15"', '$.timestamp_tz()'); -- should work
jsonb_path_query_tz
-----------------------------
- "2023-08-15T07:00:00+00:00"
+ "2023-08-15T00:00:00-07:00"
(1 row)
select jsonb_path_query('"12:34:56"', '$.timestamp_tz()');
@@ -3101,7 +3101,7 @@ HINT: Use *_tz() function for time zone support.
select jsonb_path_query_tz('"2023-08-15 12:34:56"', '$.timestamp_tz()'); -- should work
jsonb_path_query_tz
-----------------------------
- "2023-08-15T02:34:56+00:00"
+ "2023-08-15T12:34:56+10:00"
(1 row)
select jsonb_path_query('"2023-08-15 12:34:56 +05:30"', '$.timestamp_tz()');
--
2.45.2
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: jsonpath: Inconsistency of timestamp_tz() Output
@ 2024-07-19 14:05 David E. Wheeler <[email protected]>
parent: David E. Wheeler <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: David E. Wheeler @ 2024-07-19 14:05 UTC (permalink / raw)
To: Junwang Zhao <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Jul 10, 2024, at 11:19, David E. Wheeler <[email protected]> wrote:
> Oh, and the time and date were wrong, too, because I blindly used the same conversion for dates as for timestamps. Fixed in v2.
>
> PR: https://github.com/theory/postgres/pull/7
> CF: https://commitfest.postgresql.org/49/5119/
Rebase on 5784a49. No other changes. I would consider this a bug in features added for 17.
Best,
David
Attachments:
[application/octet-stream] v3-0001-Preserve-tz-when-converting-to-jsonb-timestamptz.patch (4.1K, ../../[email protected]/2-v3-0001-Preserve-tz-when-converting-to-jsonb-timestamptz.patch)
download | inline diff:
From 7f037618a85a1ac5363ba6485d0f51bb4c22137b Mon Sep 17 00:00:00 2001
From: "David E. Wheeler" <[email protected]>
Date: Wed, 10 Jul 2024 11:17:54 -0400
Subject: [PATCH v3] Preserve tz when converting to jsonb timestamptz
The JSONB jbvDatetime type has a field for offset, and displays the time
in that offset. For example, when the time zone GUC is set to
America/New_York, the jsonpath `timestamp_tz()` method returns a value
that displays a parsed value with its offset, not the local offset:
david=# set time zone 'America/New_York';
SET
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56+10"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T12:34:56+10:00"
This was not true for values parsed by `timestamp_tz()` that lacked an
offset. It correctly assumes the local time zone, but displays it in
UTC:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2024-08-15T16:34:56+00:00"
To fix this inconsistent behavior, determine the offset for values being
cast from `DATEOID` and `DATEOID` types to `jpiTimestampTz` and store it
in the resulting jbvDatetime value. With this change, the result now
preserves the offset just as it does when converting from offset-aware
values:
david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', '$.timestamp_tz()');
jsonb_path_query_tz
-----------------------------
"2023-08-15T12:34:56-04:00"
Author: David Wheeler
Reviewed-by: Junwang Zhao
Discussion: https://postgr.es/m/7DE080CE-6D8C-4794-9BD1-7D9699172FAB%40justatheory.com
---
src/backend/utils/adt/jsonpath_exec.c | 12 ++++++++++++
src/test/regress/expected/jsonb_jsonpath.out | 4 ++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index d79c929822..ac174bbaa6 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2707,12 +2707,21 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
break;
case jpiTimestampTz:
{
+ struct pg_tm tm;
+ fsec_t fsec;
/* Convert result type to timestamp with time zone */
switch (typid)
{
case DATEOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"date", "timestamptz");
+ DateADT dateVal = DatumGetDateADT(value);
+ j2date(dateVal + POSTGRES_EPOCH_JDATE,
+ &(tm.tm_year), &(tm.tm_mon), &(tm.tm_mday));
+ tm.tm_hour = 0;
+ tm.tm_min = 0;
+ tm.tm_sec = 0;
+ tz = DetermineTimeZoneOffset(&tm, session_timezone);
value = DirectFunctionCall1(date_timestamptz,
value);
break;
@@ -2726,6 +2735,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
case TIMESTAMPOID:
checkTimezoneIsUsedForCast(cxt->useTz,
"timestamp", "timestamptz");
+ if (timestamp2tm(DatumGetTimestamp(value), NULL, &tm, &fsec, NULL, NULL) == 0) {
+ tz = DetermineTimeZoneOffset(&tm, session_timezone);
+ }
value = DirectFunctionCall1(timestamp_timestamptz,
value);
break;
diff --git a/src/test/regress/expected/jsonb_jsonpath.out b/src/test/regress/expected/jsonb_jsonpath.out
index 7bb4eb1bc2..02abaac689 100644
--- a/src/test/regress/expected/jsonb_jsonpath.out
+++ b/src/test/regress/expected/jsonb_jsonpath.out
@@ -2964,7 +2964,7 @@ HINT: Use *_tz() function for time zone support.
select jsonb_path_query_tz('"2023-08-15"', '$.timestamp_tz()'); -- should work
jsonb_path_query_tz
-----------------------------
- "2023-08-15T07:00:00+00:00"
+ "2023-08-15T00:00:00-07:00"
(1 row)
select jsonb_path_query('"12:34:56"', '$.timestamp_tz()');
@@ -3151,7 +3151,7 @@ HINT: Use *_tz() function for time zone support.
select jsonb_path_query_tz('"2023-08-15 12:34:56"', '$.timestamp_tz()'); -- should work
jsonb_path_query_tz
-----------------------------
- "2023-08-15T02:34:56+00:00"
+ "2023-08-15T12:34:56+10:00"
(1 row)
select jsonb_path_query('"2023-08-15 12:34:56 +05:30"', '$.timestamp_tz()');
--
2.45.2
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-07-19 14:05 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 15:16 [PATCH v34 6/8] Allow CLUSTER and VACUUM FULL to change tablespace Alexey Kondratov <[email protected]>
2024-07-09 14:07 Re: jsonpath: Inconsistency of timestamp_tz() Output David E. Wheeler <[email protected]>
2024-07-09 14:22 ` Re: jsonpath: Inconsistency of timestamp_tz() Output David E. Wheeler <[email protected]>
2024-07-09 15:08 ` Re: jsonpath: Inconsistency of timestamp_tz() Output Junwang Zhao <[email protected]>
2024-07-09 15:38 ` Re: jsonpath: Inconsistency of timestamp_tz() Output David E. Wheeler <[email protected]>
2024-07-10 05:48 ` Re: jsonpath: Inconsistency of timestamp_tz() Output Junwang Zhao <[email protected]>
2024-07-10 14:33 ` Re: jsonpath: Inconsistency of timestamp_tz() Output David E. Wheeler <[email protected]>
2024-07-10 14:54 ` Re: jsonpath: Inconsistency of timestamp_tz() Output David E. Wheeler <[email protected]>
2024-07-10 15:19 ` Re: jsonpath: Inconsistency of timestamp_tz() Output David E. Wheeler <[email protected]>
2024-07-19 14:05 ` Re: jsonpath: Inconsistency of timestamp_tz() Output David E. Wheeler <[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