public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 1/3] Implement REINDEX of partitioned tables/indexes
6+ messages / 5 participants
[nested] [flat]
* [PATCH v4 1/3] Implement REINDEX of partitioned tables/indexes
@ 2020-06-07 20:59 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2020-06-07 20:59 UTC (permalink / raw)
---
doc/src/sgml/ref/reindex.sgml | 5 -
src/backend/catalog/index.c | 7 +-
src/backend/commands/indexcmds.c | 119 +++++++++++++--------
src/backend/tcop/utility.c | 6 +-
src/include/commands/defrem.h | 6 +-
src/test/regress/expected/create_index.out | 18 ++--
src/test/regress/sql/create_index.sql | 13 ++-
7 files changed, 106 insertions(+), 68 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index aac5d5be23..e2e32b3ba0 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -258,11 +258,6 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
can always reindex anything.
</para>
- <para>
- Reindexing partitioned tables or partitioned indexes is not supported.
- Each individual partition can be reindexed separately instead.
- </para>
-
<refsect2 id="sql-reindex-concurrently" xreflabel="Rebuilding Indexes Concurrently">
<title>Rebuilding Indexes Concurrently</title>
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1be27eec52..7abd7f5cfc 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3660,12 +3660,7 @@ reindex_relation(Oid relid, int flags, int options)
*/
rel = table_open(relid, ShareLock);
- /*
- * This may be useful when implemented someday; but that day is not today.
- * For now, avoid erroring out when called in a multi-table context
- * (REINDEX SCHEMA) and happen to come across a partitioned table. The
- * partitions may be reindexed on their own anyway.
- */
+ /* Avoid erroring out */
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
ereport(WARNING,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2baca12c5f..df3e567acb 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -88,7 +88,8 @@ static List *ChooseIndexColumnNames(List *indexElems);
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
static bool ReindexRelationConcurrently(Oid relationOid, int options);
-static void ReindexPartitionedIndex(Relation parentIdx);
+static void ReindexPartitionedRel(Oid reloid, int options, bool concurrent,
+ bool isTopLevel);
static void update_relispartition(Oid relationId, bool newval);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
@@ -2420,11 +2421,10 @@ ChooseIndexColumnNames(List *indexElems)
* Recreate a specific index.
*/
void
-ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
+ReindexIndex(RangeVar *indexRelation, int options, bool concurrent, bool isTopLevel)
{
struct ReindexIndexCallbackState state;
Oid indOid;
- Relation irel;
char persistence;
/*
@@ -2445,22 +2445,10 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
RangeVarCallbackForReindexIndex,
&state);
- /*
- * Obtain the current persistence of the existing index. We already hold
- * lock on the index.
- */
- irel = index_open(indOid, NoLock);
-
- if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- {
- ReindexPartitionedIndex(irel);
- return;
- }
-
- persistence = irel->rd_rel->relpersistence;
- index_close(irel, NoLock);
-
- if (concurrent && persistence != RELPERSISTENCE_TEMP)
+ persistence = get_rel_persistence(indOid);
+ if (get_rel_relkind(indOid) == RELKIND_PARTITIONED_INDEX)
+ ReindexPartitionedRel(indOid, options, concurrent, isTopLevel);
+ else if (concurrent && persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
reindex_index(indOid, false, persistence,
@@ -2542,7 +2530,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* Recreate all indexes of a table (and of its toast table, if any)
*/
Oid
-ReindexTable(RangeVar *relation, int options, bool concurrent)
+ReindexTable(RangeVar *relation, int options, bool concurrent, bool isTopLevel)
{
Oid heapOid;
bool result;
@@ -2560,7 +2548,9 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
0,
RangeVarCallbackOwnsTable, NULL);
- if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
+ if (get_rel_relkind(heapOid) == RELKIND_PARTITIONED_TABLE)
+ ReindexPartitionedRel(heapOid, options, concurrent, isTopLevel);
+ else if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
{
result = ReindexRelationConcurrently(heapOid, options);
@@ -2688,11 +2678,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
* Only regular tables and matviews can have indexes, so ignore any
* other kind of relation.
*
- * It is tempting to also consider partitioned tables here, but that
- * has the problem that if the children are in the same schema, they
- * would be processed twice. Maybe we could have a separate list of
- * partitioned tables, and expand that afterwards into relids,
- * ignoring any duplicates.
+ * Partitioned tables/indexes are skipped but matching leaf
+ * partitions are processed.
*/
if (classtuple->relkind != RELKIND_RELATION &&
classtuple->relkind != RELKIND_MATVIEW)
@@ -2805,8 +2792,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
* view. For tables and materialized views, all its indexes will be rebuilt,
* excluding invalid indexes and any indexes used in exclusion constraints,
* but including its associated toast table indexes. For indexes, the index
- * itself will be rebuilt. If 'relationOid' belongs to a partitioned table
- * then we issue a warning to mention these are not yet supported.
+ * itself will be rebuilt.
*
* The locks taken on parent tables and involved indexes are kept until the
* transaction is committed, at which point a session lock is taken on each
@@ -3010,13 +2996,6 @@ ReindexRelationConcurrently(Oid relationOid, int options)
MemoryContextSwitchTo(oldcontext);
break;
}
- case RELKIND_PARTITIONED_TABLE:
- /* see reindex_relation() */
- ereport(WARNING,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
- get_rel_name(relationOid))));
- return false;
default:
/* Return error if type of relation is not supported */
ereport(ERROR,
@@ -3478,17 +3457,71 @@ ReindexRelationConcurrently(Oid relationOid, int options)
}
/*
- * ReindexPartitionedIndex
- * Reindex each child of the given partitioned index.
- *
- * Not yet implemented.
+ * ReindexPartitionedRel
+ * Reindex each child of the given partitioned relation.
*/
static void
-ReindexPartitionedIndex(Relation parentIdx)
+ReindexPartitionedRel(Oid reloid, int options, bool concurrent, bool isTopLevel)
{
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("REINDEX is not yet implemented for partitioned indexes")));
+ MemoryContext oldcontext, reindex_context;
+ List *inhoids;
+ ListCell *lc;
+
+ /*
+ * This cannot run inside a user transaction block; if
+ * we were inside a transaction, then its commit- and
+ * start-transaction-command calls would not have the
+ * intended effect!
+ */
+ PreventInTransactionBlock(isTopLevel,
+ "REINDEX of partitioned relations"); // XXX
+
+ /*
+ * Create list of children in longlived context, since we process each
+ * child in a separate transaction
+ */
+ reindex_context = AllocSetContextCreate(PortalContext, "Reindex",
+ ALLOCSET_DEFAULT_SIZES);
+ oldcontext = MemoryContextSwitchTo(reindex_context);
+ inhoids = find_all_inheritors(reloid, NoLock, NULL);
+ MemoryContextSwitchTo(oldcontext);
+
+ foreach (lc, inhoids)
+ {
+ Oid inhrelid = lfirst_oid(lc);
+
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+
+ StartTransactionCommand();
+ /* functions in indexes may want a snapshot set */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
+ switch (get_rel_relkind(inhrelid))
+ {
+ case RELKIND_PARTITIONED_INDEX:
+ case RELKIND_PARTITIONED_TABLE:
+ /*
+ * We have a full list of direct and indirect children, so skip
+ * partitioned relations and just handle their children.
+ */
+ continue;
+ case RELKIND_INDEX:
+ reindex_index(inhrelid, false, get_rel_persistence(inhrelid),
+ options | REINDEXOPT_REPORT_PROGRESS);
+ break;
+ case RELKIND_RELATION:
+ (void) reindex_relation(inhrelid,
+ REINDEX_REL_PROCESS_TOAST |
+ REINDEX_REL_CHECK_CONSTRAINTS,
+ options | REINDEXOPT_REPORT_PROGRESS);
+ break;
+ }
+ }
+
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ StartTransactionCommand();
}
/*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 9b0c376c8c..fd6bc65c18 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -926,10 +926,12 @@ standard_ProcessUtility(PlannedStmt *pstmt,
switch (stmt->kind)
{
case REINDEX_OBJECT_INDEX:
- ReindexIndex(stmt->relation, stmt->options, stmt->concurrent);
+ ReindexIndex(stmt->relation, stmt->options,
+ stmt->concurrent, isTopLevel);
break;
case REINDEX_OBJECT_TABLE:
- ReindexTable(stmt->relation, stmt->options, stmt->concurrent);
+ ReindexTable(stmt->relation, stmt->options,
+ stmt->concurrent, isTopLevel);
break;
case REINDEX_OBJECT_SCHEMA:
case REINDEX_OBJECT_SYSTEM:
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index c26a102b17..df32f5b201 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -34,8 +34,10 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool check_not_in_use,
bool skip_build,
bool quiet);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent);
-extern Oid ReindexTable(RangeVar *relation, int options, bool concurrent);
+extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent,
+ bool isTopLevel);
+extern Oid ReindexTable(RangeVar *relation, int options, bool concurrent,
+ bool isTopLevel);
extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
int options, bool concurrent);
extern char *makeObjectName(const char *name1, const char *name2,
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index e3e6634d7e..61b4a30db4 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2196,18 +2196,20 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
concur_reindex_part_index_0_2 | concur_reindex_part_index_0 | 2
(5 rows)
--- REINDEX fails for partitioned indexes
+-- REINDEX for partitioned indexes
+REINDEX INDEX concur_reindex_part_index;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+REINDEX INDEX concur_reindex_part_index_0;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index_0;
REINDEX INDEX concur_reindex_part_index_10;
-ERROR: REINDEX is not yet implemented for partitioned indexes
REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
-ERROR: REINDEX is not yet implemented for partitioned indexes
--- REINDEX is a no-op for partitioned tables
+-- REINDEX for partitioned tables
+REINDEX TABLE concur_reindex_part_10;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
+REINDEX TABLE concur_reindex_part_0;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_0;
REINDEX TABLE concur_reindex_part_10;
-WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE: table "concur_reindex_part_10" has no indexes to reindex
REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
-WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE: table "concur_reindex_part_10" has no indexes that can be reindexed concurrently
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
relid | parentrelid | level
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f3667bacdc..a751dd5caa 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -903,12 +903,21 @@ CREATE INDEX concur_reindex_part_index_0_2 ON ONLY concur_reindex_part_0_2 (c1);
ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_part_index_0_2;
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
--- REINDEX fails for partitioned indexes
+-- REINDEX for partitioned indexes
+REINDEX INDEX concur_reindex_part_index;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+REINDEX INDEX concur_reindex_part_index_0;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index_0;
REINDEX INDEX concur_reindex_part_index_10;
REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
--- REINDEX is a no-op for partitioned tables
+-- REINDEX for partitioned tables
REINDEX TABLE concur_reindex_part_10;
REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
+REINDEX TABLE concur_reindex_part_0;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_0;
+REINDEX TABLE concur_reindex_part_10;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
+
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
-- REINDEX should preserve dependencies of partition tree.
--
2.17.0
--JYK4vJDZwFMowpUq
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0002-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Patch: Add parse_type Function
@ 2024-02-05 15:14 David E. Wheeler <[email protected]>
2024-02-05 15:47 ` Re: Patch: Add parse_type Function Jim Jones <[email protected]>
2024-02-11 00:00 ` Re: Patch: Add parse_type Function jian he <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: David E. Wheeler @ 2024-02-05 15:14 UTC (permalink / raw)
To: Jim Jones <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; [email protected]; Pavel Stehule <[email protected]>
On Feb 5, 2024, at 09:49, Jim Jones <[email protected]> wrote:
> Yes, if the function was strict (which in the current design is not the
> case) there is no need to check for nulls.
Right, done, and cleaned up the redundant comments.
Best,
David
Attachments:
[application/octet-stream] v3-0001-Add-parse_type-SQL-function.patch (10.2K, ../../[email protected]/2-v3-0001-Add-parse_type-SQL-function.patch)
download | inline diff:
From c514e2900bf4967186c13922bcb9808dcfd3f851 Mon Sep 17 00:00:00 2001
From: "David E. Wheeler" <[email protected]>
Date: Mon, 5 Feb 2024 10:12:40 -0500
Subject: [PATCH v3] Add parse_type() SQL function
The `parse_type()` function uses the underlying `parseTypeString()` C
function to parse a string representing a data type into a type ID and
typmod suitabld for passing to `format_type()`. This allows one to
derive the formal SQL name for a type from a string that may be an
alias:
SELECT format_type(p.typid, p.typmod)
FROM parse_type('timestamp(4)') p;
format_type
--------------------------------
timestamp(4) without time zone
Also accounts for data typs that require the SQL grammar to be parsed:
SELECT format_type(p.typid, p.typmod)
FROM parse_type('interval second(0)') p;
format_type
--------------------
interval second(0)
Useful for unit tests for against column data types, for example.
Originally written by Erik Wienhold for use in pgTAP.
---
doc/src/sgml/func.sgml | 31 +++++++++++-
src/backend/utils/adt/format_type.c | 59 +++++++++++++++++++++++
src/include/catalog/pg_proc.dat | 4 ++
src/include/utils/builtins.h | 2 +
src/test/regress/expected/create_type.out | 53 ++++++++++++++++++++
src/test/regress/sql/create_type.sql | 28 +++++++++++
6 files changed, 176 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6788ba8ef4..1e2426acae 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24754,7 +24754,7 @@ SELECT pg_type_is_visible('myschema.widget'::regtype);
<tbody>
<row>
- <entry role="func_table_entry"><para role="func_signature">
+ <entry id="format_type" role="func_table_entry"><para role="func_signature">
<indexterm>
<primary>format_type</primary>
</indexterm>
@@ -24768,6 +24768,35 @@ SELECT pg_type_is_visible('myschema.widget'::regtype);
</para></entry>
</row>
+ <row>
+ <entry id="parse_type" role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>parse_type</primary>
+ </indexterm>
+ <function>parse_type</function> ( <parameter>type</parameter> <type>text</type> )
+ <returnvalue>record</returnvalue>
+ ( <parameter>typid</parameter> <type>oid</type>,
+ <parameter>typmod</parameter> <type>int4</type> )
+ </para>
+ <para>
+ Parses a string representing an SQL type declaration as used in a
+ <command>CREATE TABLE</command> statement, optionally schema-qualified.
+ Returns a record with two fields, <parameter>typid</parameter> and
+ <parameter>typmod</parameter>, representing the OID and modifier for the
+ type. These correspond to the parameters to pass to the
+ <link linkend="format_type"><function>format_type</function> function.</link>
+ </para>
+ <para>
+ For example:
+<programlisting>
+SELECT format_type(p.typid, p.typmod) FROM parse_type('timestamp(4)') p;
+ format_type
+ --------------------------------
+ timestamp(4) without time zone
+</programlisting>
+ </para></entry>
+ </row>
+
<row>
<entry id="pg-char-to-encoding" role="func_table_entry"><para role="func_signature">
<indexterm>
diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index 28ba0fbd19..0d56c8cbb1 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -26,6 +26,9 @@
#include "utils/lsyscache.h"
#include "utils/numeric.h"
#include "utils/syscache.h"
+#include "fmgr.h"
+#include "funcapi.h"
+#include "parser/parse_type.h"
static char *printTypmod(const char *typname, int32 typmod, Oid typmodout);
@@ -482,3 +485,59 @@ oidvectortypes(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+
+/*
+ * parse_type() is the inverse of pg_catalog.format_type(): it takes a string
+ * representing an SQL-compatible type declaration, such as "int4" or "integer"
+ * or "character varying(32)", parses it, and returns the OID and type modifier.
+ *
+ * Raises an error on an invalid type.
+ *
+ * Internally it relies on the Postgres core parseTypeString() function defined
+ * in src/backend/parser/parse_type.c.
+ */
+PG_FUNCTION_INFO_V1(parse_type);
+
+Datum
+parse_type(PG_FUNCTION_ARGS)
+{
+#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */
+ const char *type; /* the type string we want to resolve */
+ Oid typid; /* the resolved type oid */
+ int32 typmod; /* the resolved type modifier */
+ TupleDesc tupdesc;
+ HeapTuple rettuple;
+ Datum values[PARSE_TYPE_STRING_COLS] = {0};
+ bool nulls[PARSE_TYPE_STRING_COLS] = {0};
+
+ type = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+ /*
+ * Build a tuple descriptor for our result type; return an error if not
+ * called in a context that expects a record.
+ */
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) {
+ ereport(
+ ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("function returning record called in context that cannot accept type record"))
+ );
+ }
+
+ BlessTupleDesc(tupdesc);
+
+ /*
+ * Parse type-name argument to obtain type OID and encoded typmod. We don't
+ * need to check for parseTypeString failure, but just let the error be
+ * raised. The 0 arg works both as the `Node *escontext` arg in Postgres 16
+ * and the `bool missing_ok` arg in 9.4-15.
+ */
+ (void) parseTypeString(type, &typid, &typmod, 0);
+
+ /* Create and return tuple. */
+ values[0] = typid;
+ values[1] = typmod;
+ rettuple = heap_form_tuple(tupdesc, values, nulls);
+ return HeapTupleGetDatum(rettuple);
+#undef PARSE_TYPE_STRING_COLS
+}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 29af4ce65d..37205e1b31 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -2184,6 +2184,10 @@
{ oid => '1081', descr => 'format a type oid and atttypmod to canonical SQL',
proname => 'format_type', proisstrict => 'f', provolatile => 's',
prorettype => 'text', proargtypes => 'oid int4', prosrc => 'format_type' },
+{ oid => '8401', descr => 'parse a type string into its a type oid and atttypmod',
+ proname => 'parse_type', proisstrict => 't', provolatile => 's',
+ prorettype => 'record', proargtypes => 'text', prosrc => 'parse_type',
+ proallargtypes => '{text,oid,int4}', proargmodes => '{i,o,o}', proargnames => '{typname,typid,typmod}' },
{ oid => '1084', descr => 'I/O',
proname => 'date_in', provolatile => 's', prorettype => 'date',
proargtypes => 'cstring', prosrc => 'date_in' },
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 359c570f23..264ad090be 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -133,6 +133,8 @@ extern char *format_type_with_typemod(Oid type_oid, int32 typemod);
extern int32 type_maximum_size(Oid type_oid, int32 typemod);
+extern Datum parse_type(PG_FUNCTION_ARGS);
+
/* quote.c */
extern char *quote_literal_cstr(const char *rawstr);
diff --git a/src/test/regress/expected/create_type.out b/src/test/regress/expected/create_type.out
index 7383fcdbb1..84ea71090c 100644
--- a/src/test/regress/expected/create_type.out
+++ b/src/test/regress/expected/create_type.out
@@ -249,6 +249,59 @@ select format_type('bpchar'::regtype, -1);
bpchar
(1 row)
+-- Test parse_type
+SELECT * FROM parse_type('text') p(typid, typmod);
+ typid | typmod
+-------+--------
+ 25 | -1
+(1 row)
+
+SELECT * FROM parse_type(NULL) p(typid, typmod);
+ typid | typmod
+-------+--------
+ |
+(1 row)
+
+-- Test parse_type errors
+SELECT parse_type('nonesuch'); -- error expected
+ERROR: type "nonesuch" does not exist
+SELECT parse_type('interval nonesuch'); -- grammar error expected
+ERROR: syntax error at or near "nonesuch"
+LINE 1: SELECT parse_type('interval nonesuch');
+ ^
+CONTEXT: invalid type name "interval nonesuch"
+SELECT parse_type('year(4)'); -- grammar error expected
+ERROR: type "year" does not exist
+-- Test parse_type with various aliases and grammar-based types
+WITH s(s) AS (
+ SELECT * FROM unnest(ARRAY[
+ 'timestamp(4)',
+ 'interval(0)',
+ 'interval second(0)',
+ 'timestamptz',
+ 'timestamptz(6)',
+ 'varchar',
+ 'varchar(128)',
+ 'mytab'
+ ])
+),
+p(typid, typmod) AS (
+ SELECT ((parse_type(s)).*)
+ FROM s
+)
+SELECT format_type(typid, typmod) FROM p;
+ format_type
+--------------------------------
+ timestamp(4) without time zone
+ interval(0)
+ interval second(0)
+ timestamp with time zone
+ timestamp(6) with time zone
+ character varying
+ character varying(128)
+ mytab
+(8 rows)
+
-- Test non-error-throwing APIs using widget, which still throws errors
SELECT pg_input_is_valid('(1,2,3)', 'widget');
pg_input_is_valid
diff --git a/src/test/regress/sql/create_type.sql b/src/test/regress/sql/create_type.sql
index c25018029c..e4700b8719 100644
--- a/src/test/regress/sql/create_type.sql
+++ b/src/test/regress/sql/create_type.sql
@@ -192,6 +192,34 @@ select format_type('bpchar'::regtype, null);
-- this behavior difference is intentional
select format_type('bpchar'::regtype, -1);
+-- Test parse_type
+SELECT * FROM parse_type('text') p(typid, typmod);
+SELECT * FROM parse_type(NULL) p(typid, typmod);
+
+-- Test parse_type errors
+SELECT parse_type('nonesuch'); -- error expected
+SELECT parse_type('interval nonesuch'); -- grammar error expected
+SELECT parse_type('year(4)'); -- grammar error expected
+
+-- Test parse_type with various aliases and grammar-based types
+WITH s(s) AS (
+ SELECT * FROM unnest(ARRAY[
+ 'timestamp(4)',
+ 'interval(0)',
+ 'interval second(0)',
+ 'timestamptz',
+ 'timestamptz(6)',
+ 'varchar',
+ 'varchar(128)',
+ 'mytab'
+ ])
+),
+p(typid, typmod) AS (
+ SELECT ((parse_type(s)).*)
+ FROM s
+)
+SELECT format_type(typid, typmod) FROM p;
+
-- Test non-error-throwing APIs using widget, which still throws errors
SELECT pg_input_is_valid('(1,2,3)', 'widget');
SELECT pg_input_is_valid('(1,2)', 'widget'); -- hard error expected
--
2.43.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Patch: Add parse_type Function
2024-02-05 15:14 Re: Patch: Add parse_type Function David E. Wheeler <[email protected]>
@ 2024-02-05 15:47 ` Jim Jones <[email protected]>
2024-02-05 15:56 ` Re: Patch: Add parse_type Function David E. Wheeler <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Jim Jones @ 2024-02-05 15:47 UTC (permalink / raw)
To: David E. Wheeler <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; [email protected]; Pavel Stehule <[email protected]>
On 05.02.24 16:14, David E. Wheeler wrote:
> Right, done, and cleaned up the redundant comments.
>
> Best,
>
> David
Nice.
The patch applies cleanly and it no longer crashes with NULL parameters.
Docs render fine and regression tests pass. I have nothing more to add.
Let's now wait for Pavel's review.
Thanks!
--
Jim
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Patch: Add parse_type Function
2024-02-05 15:14 Re: Patch: Add parse_type Function David E. Wheeler <[email protected]>
2024-02-05 15:47 ` Re: Patch: Add parse_type Function Jim Jones <[email protected]>
@ 2024-02-05 15:56 ` David E. Wheeler <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: David E. Wheeler @ 2024-02-05 15:56 UTC (permalink / raw)
To: Jim Jones <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; [email protected]; Pavel Stehule <[email protected]>
On Feb 5, 2024, at 10:47 AM, Jim Jones <[email protected]> wrote:
> The patch applies cleanly and it no longer crashes with NULL parameters.
> Docs render fine and regression tests pass. I have nothing more to add.
> Let's now wait for Pavel's review.
Much obliged!
D
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Patch: Add parse_type Function
2024-02-05 15:14 Re: Patch: Add parse_type Function David E. Wheeler <[email protected]>
@ 2024-02-11 00:00 ` jian he <[email protected]>
2024-02-11 01:52 ` Re: Patch: Add parse_type Function Erik Wienhold <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: jian he @ 2024-02-11 00:00 UTC (permalink / raw)
To: David E. Wheeler <[email protected]>; +Cc: Jim Jones <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; [email protected]; Pavel Stehule <[email protected]>
+ /*
+ * Parse type-name argument to obtain type OID and encoded typmod. We don't
+ * need to check for parseTypeString failure, but just let the error be
+ * raised. The 0 arg works both as the `Node *escontext` arg in Postgres 16
+ * and the `bool missing_ok` arg in 9.4-15.
+ */
+ (void) parseTypeString(type, &typid, &typmod, 0);
if you are wondering around other code deal with NULL, ErrorSaveContext.
NULL would be more correct?
`(void) parseTypeString(type, &typid, &typmod, NULL);`
also parseTypeString already explained the third argument, our
comments can be simplified as:
/*
* Parse type-name argument to obtain type OID and encoded typmod. We don't
* need to handle parseTypeString failure, just let the error be
* raised.
*/
cosmetic issue. Looking around other error handling code, the format
should be something like:
`
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("function returning record called in"
"context that cannot accept type record")));
`
`PG_FUNCTION_INFO_V1(parse_type);`
is unnecessary?
based on the error information: https://cirrus-ci.com/task/5899457502380032
not sure why it only fails on windows.
+#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */
+#undef PARSE_TYPE_STRING_COLS
Are these necessary?
given that the comments already say return the OID and type modifier.
+ ( <parameter>typid</parameter> <type>oid</type>,
+ <parameter>typmod</parameter> <type>int4</type> )
here `int4` should be `integer`?
commit message:
`Also accounts for data typs that require the SQL grammar to be parsed:`
except the typo issue, this sentence is still hard for me to understand.
The `parse_type()` function uses the underlying `parseTypeString()` C
function to parse a string representing a data type into a type ID and
typmod suitabld for passing to `format_type()`.
suitabld should be suitable.
+ <para>
+ Parses a string representing an SQL type declaration as used in a
+ <command>CREATE TABLE</command> statement, optionally schema-qualified.
+ Returns a record with two fields, <parameter>typid</parameter> and
+ <parameter>typmod</parameter>, representing the OID and
modifier for the
+ type. These correspond to the parameters to pass to the
+ <link linkend="format_type"><function>format_type</function>
function.</link>
+ </para>
can be simplified:
+ <para>
+ Parses a string representing an SQL data type, optionally
schema-qualified.
+ Returns a record with two fields, <parameter>typid</parameter> and
+ <parameter>typmod</parameter>, representing the OID and
modifier for the
+ type. These correspond to the parameters to pass to the
+ <link linkend="format_type"><function>format_type</function>
function.</link>
+ </para>
(I don't have a strong opinion though)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Patch: Add parse_type Function
2024-02-05 15:14 Re: Patch: Add parse_type Function David E. Wheeler <[email protected]>
2024-02-11 00:00 ` Re: Patch: Add parse_type Function jian he <[email protected]>
@ 2024-02-11 01:52 ` Erik Wienhold <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Erik Wienhold @ 2024-02-11 01:52 UTC (permalink / raw)
To: jian he <[email protected]>; David E. Wheeler <[email protected]>; +Cc: Jim Jones <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; [email protected]; Pavel Stehule <[email protected]>
Let me comment on some issues since I wrote the very first version of
parse_type() on which David's patch is based.
> On 2024-02-01 01:00 +0100, jian he <[email protected]> wrote:
>
> cosmetic issue. Looking around other error handling code, the format
> should be something like:
> `
> if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("function returning record called in"
> "context that cannot accept type record")));
> `
+1
> `PG_FUNCTION_INFO_V1(parse_type);`
> is unnecessary?
> based on the error information: https://cirrus-ci.com/task/5899457502380032
> not sure why it only fails on windows.
Yeah, it's not necessary for internal functions per [1]. It's a
leftover from when this function was part of the pgtap extension.
> +#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */
> +#undef PARSE_TYPE_STRING_COLS
> Are these necessary?
> given that the comments already say return the OID and type modifier.
Not sure what the convention here is but I found this in a couple of
places, maybe even a tutorial on writing C functions. See
`git grep '_COLS\s\+[1-9]'` for those instances. It's in line with my
habit of avoiding magic constants.
> + ( <parameter>typid</parameter> <type>oid</type>,
> + <parameter>typmod</parameter> <type>int4</type> )
> here `int4` should be `integer`?
+1
> commit message:
> `Also accounts for data typs that require the SQL grammar to be parsed:`
> except the typo issue, this sentence is still hard for me to understand.
Yes, the sentence is rather handwavy. What is meant here is that this
function also resolves types whose typmod is determined by the SQL
parser or some step after that. There are types whose typmod is
whatever value is found inside the parenthesis, e.g. bit(13) has typmod
13. Our first idea before coming up with that function was to do some
string manipulation and extract the typmod from inside the parenthesis.
But we soon found out that other typmods don't translate one to one,
e.g. varchar(13) has typmod 17. The interval type is also special
because the typmod is some bitmask encoding of e.g. 'second(0)'. Hence
the mention of the SQL grammar.
> + <para>
> + Parses a string representing an SQL type declaration as used in a
> + <command>CREATE TABLE</command> statement, optionally schema-qualified.
> + Returns a record with two fields, <parameter>typid</parameter> and
> + <parameter>typmod</parameter>, representing the OID and
> modifier for the
> + type. These correspond to the parameters to pass to the
> + <link linkend="format_type"><function>format_type</function>
> function.</link>
> + </para>
>
> can be simplified:
> + <para>
> + Parses a string representing an SQL data type, optionally
> schema-qualified.
> + Returns a record with two fields, <parameter>typid</parameter> and
> + <parameter>typmod</parameter>, representing the OID and
> modifier for the
> + type. These correspond to the parameters to pass to the
> + <link linkend="format_type"><function>format_type</function>
> function.</link>
> + </para>
> (I don't have a strong opinion though)
Sounds better. The CREATE TABLE reference is superfluous.
[1] https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-C-V1-CALL-CONV
--
Erik
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-02-11 01:52 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-06-07 20:59 [PATCH v4 1/3] Implement REINDEX of partitioned tables/indexes Justin Pryzby <[email protected]>
2024-02-05 15:14 Re: Patch: Add parse_type Function David E. Wheeler <[email protected]>
2024-02-05 15:47 ` Re: Patch: Add parse_type Function Jim Jones <[email protected]>
2024-02-05 15:56 ` Re: Patch: Add parse_type Function David E. Wheeler <[email protected]>
2024-02-11 00:00 ` Re: Patch: Add parse_type Function jian he <[email protected]>
2024-02-11 01:52 ` Re: Patch: Add parse_type Function Erik Wienhold <[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