On Wed, Oct 11, 2023 at 12:47 PM Paul Jungwirth
<p...@illuminatedcomputing.com> wrote:
>
> On 9/25/23 14:00, Peter Eisentraut wrote:
> > Looking through the tests in v16-0001:
> >
> > +-- PK with no columns just WITHOUT OVERLAPS:
> > +CREATE TABLE temporal_rng (
> > + valid_at tsrange,
> > + CONSTRAINT temporal_rng_pk PRIMARY KEY (valid_at WITHOUT OVERLAPS)
> > +);
> > +ERROR: syntax error at or near "WITHOUT"
> > +LINE 3: CONSTRAINT temporal_rng_pk PRIMARY KEY (valid_at WITHOUT OV...
> > + ^
> >
> > I think this error is confusing. The SQL standard requires at least one
> > non-period column in a PK. I don't know why that is or why we should
> > implement it. But if we want to implement it, maybe we should enforce
> > that in parse analysis rather than directly in the parser, to be able to
> > produce a more friendly error message.
>
> Okay.
>
> (I think the reason the standard requires one non-period column is to
> identify the "entity". If philosophically the row is an Aristotelian
> proposition about that thing, the period qualifies it as true just
> during some time span. So the scalar part is doing the work that a PK
> conventionally does, and the period part does something else. Perhaps a
> PK/UNIQUE constraint with no scalar part would still be useful, but not
> very often I think, and I'm not sure it makes sense to call it PK/UNIQUE.)
>
> > +-- PK with a range column/PERIOD that isn't there:
> > +CREATE TABLE temporal_rng (
> > + id INTEGER,
> > + CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT
> > OVERLAPS)
> > +);
> > +ERROR: range or PERIOD "valid_at" in WITHOUT OVERLAPS does not exist
> >
> > I think here we should just produce a "column doesn't exist" error
> > message, the same as if the "id" column was invalid. We don't need to
> > get into the details of what kind of column it should be. That is done
> > in the next test
>
> I'll change it. The reason for the different wording is that it might
> not be a column at all. It might be a PERIOD. So what about just "column
> or PERIOD doesn't exist"? (Your suggestion is fine too though.)
>
> > +ERROR: column "valid_at" in WITHOUT OVERLAPS is not a range type
> >
> > Also, in any case it would be nice to have a location pointer here (for
> > both cases).
>
> Agreed.
>
I refactored findNeworOldColumn to better handle error reports.
please check the attached.
From 81a5eab2596c0e322dee8165ad73343328f496ed Mon Sep 17 00:00:00 2001
From: pgaddict <jian.universal...@gmail.com>
Date: Wed, 25 Oct 2023 13:50:38 +0800
Subject: [PATCH v1 1/1] refactor findNewOrOldColumn.
refator findNeworOldColumn to validate WITHOUT OVERLAP constraint
associated key column exists and it's a range data type.
create/alter table add without overlap constraint:
if the key column not exists,
it will report the same as no "without overlap" normal case.
create table add without overlap constraint:
if overlap constraint associated column's data type is not range,
it will report it's not a range error, also print out the
parser position.
alter table add without overlap constraint:
if overlap constraint associated column's data type is not range,
it will report not a range data type error.
it will not print out parser position (I don't know how to do that).
---
src/backend/parser/parse_utilcmd.c | 81 ++++++++++---------
.../regress/expected/without_overlaps.out | 8 +-
2 files changed, 47 insertions(+), 42 deletions(-)
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index e195410c..f44de591 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -124,8 +124,7 @@ static List *get_opclass(Oid opclass, Oid actual_datatype);
static void transformIndexConstraints(CreateStmtContext *cxt);
static IndexStmt *transformIndexConstraint(Constraint *constraint,
CreateStmtContext *cxt);
-static bool findNewOrOldColumn(CreateStmtContext *cxt, char *colname, char **typname,
- Oid *typid);
+static void validate_without_overlaps(CreateStmtContext *cxt, char *colname);
static void transformExtendedStatistics(CreateStmtContext *cxt);
static void transformFKConstraints(CreateStmtContext *cxt,
bool skipValidation,
@@ -2698,34 +2697,19 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
* Anything in without_overlaps should be included,
* but with the overlaps operator (&&) instead of equality.
*/
- if (constraint->without_overlaps != NULL) {
+ if (constraint->without_overlaps != NULL)
+ {
char *without_overlaps_str = strVal(constraint->without_overlaps);
IndexElem *iparam = makeNode(IndexElem);
- char *typname;
- Oid typid;
/*
* Iterate through the table's columns
* (like just a little bit above).
* If we find one whose name is the same as without_overlaps,
* validate that it's a range type.
- *
- * Otherwise report an error.
+ * if it cannot found, raise an error.
*/
-
- if (findNewOrOldColumn(cxt, without_overlaps_str, &typname, &typid))
- {
- if (!type_is_range(typid))
- ereport(ERROR,
- (errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("column \"%s\" in WITHOUT OVERLAPS is not a range type",
- without_overlaps_str)));
- }
- else
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("range or PERIOD \"%s\" in WITHOUT OVERLAPS does not exist",
- without_overlaps_str)));
+ validate_without_overlaps(cxt, without_overlaps_str);
iparam->name = pstrdup(without_overlaps_str);
iparam->expr = NULL;
@@ -2744,6 +2728,9 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
{
/*
* Force the column to NOT NULL since it is part of the primary key.
+ * prior loop foreach(lc, constraint->keys) does not loop over
+ * without_overlap keys. So we need manually add here.
+ *
*/
AlterTableCmd *notnullcmd = makeNode(AlterTableCmd);
@@ -2872,31 +2859,37 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
}
/*
- * Tries to find a column by name among the existing ones (if it's an ALTER TABLE)
- * and the new ones. Sets typname and typid if one is found. Returns false if we
- * couldn't find a match.
+ * Tries to find and validate the typid associated
+ * with a without_overlaps column name, even in ALTER TABLE case.
+ * After check, without_overlaps column should exist, and it's a range type.
+ *
*/
-static bool
-findNewOrOldColumn(CreateStmtContext *cxt, char *colname, char **typname, Oid *typid)
+static void
+validate_without_overlaps(CreateStmtContext *cxt, char *colname)
{
/* Check the new columns first in case their type is changing. */
-
ColumnDef *column = NULL;
ListCell *columns;
+ Oid typid;
+ bool found = false;
foreach(columns, cxt->columns)
{
column = lfirst_node(ColumnDef, columns);
if (strcmp(column->colname, colname) == 0)
{
- *typid = typenameTypeId(NULL, column->typeName);
- *typname = TypeNameToString(column->typeName);
- return true;
+ typid = typenameTypeId(NULL, column->typeName);
+ if (!type_is_range(typid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("column \"%s\" in WITHOUT OVERLAPS is not a range type",colname),
+ parser_errposition(cxt->pstate, column->location)));
+ found = true;
+ break;
}
}
/* Look up columns on existing table. */
-
if (cxt->isalter)
{
Relation rel = cxt->rel;
@@ -2905,22 +2898,30 @@ findNewOrOldColumn(CreateStmtContext *cxt, char *colname, char **typname, Oid *t
Form_pg_attribute attr = TupleDescAttr(rel->rd_att, i);
const char *attname;
- if (attr->attisdropped)
- continue;
-
attname = NameStr(attr->attname);
if (strcmp(attname, colname) == 0)
{
- Type type = typeidType(attr->atttypid);
- *typid = attr->atttypid;
- *typname = pstrdup(typeTypeName(type));
- ReleaseSysCache(type);
- return true;
+ typid = attr->atttypid;
+
+ if (attr->attisdropped)
+ {
+ found = false;
+ break;
+ }
+ if (!type_is_range(typid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("column \"%s\" in WITHOUT OVERLAPS is not a range type", colname)));
+ found = true;
+ break;
}
}
}
- return false;
+ if (!found)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" named in key does not exist",colname)));
}
/*
diff --git a/src/test/regress/expected/without_overlaps.out b/src/test/regress/expected/without_overlaps.out
index 4b8abb08..a916e56a 100644
--- a/src/test/regress/expected/without_overlaps.out
+++ b/src/test/regress/expected/without_overlaps.out
@@ -19,7 +19,7 @@ CREATE TABLE temporal_rng (
id INTEGER,
CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
);
-ERROR: range or PERIOD "valid_at" in WITHOUT OVERLAPS does not exist
+ERROR: column "valid_at" named in key does not exist
-- PK with a non-range column:
CREATE TABLE temporal_rng (
id INTEGER,
@@ -27,6 +27,8 @@ CREATE TABLE temporal_rng (
CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
);
ERROR: column "valid_at" in WITHOUT OVERLAPS is not a range type
+LINE 3: valid_at TEXT,
+ ^
-- PK with one column plus a range:
CREATE TABLE temporal_rng (
-- Since we can't depend on having btree_gist here,
@@ -110,7 +112,7 @@ CREATE TABLE temporal_rng2 (
id INTEGER,
CONSTRAINT temporal_rng2_uq UNIQUE (id, valid_at WITHOUT OVERLAPS)
);
-ERROR: range or PERIOD "valid_at" in WITHOUT OVERLAPS does not exist
+ERROR: column "valid_at" named in key does not exist
-- UNIQUE with a non-range column:
CREATE TABLE temporal_rng2 (
id INTEGER,
@@ -118,6 +120,8 @@ CREATE TABLE temporal_rng2 (
CONSTRAINT temporal_rng2_uq UNIQUE (id, valid_at WITHOUT OVERLAPS)
);
ERROR: column "valid_at" in WITHOUT OVERLAPS is not a range type
+LINE 3: valid_at TEXT,
+ ^
-- UNIQUE with one column plus a range:
CREATE TABLE temporal_rng2 (
-- Since we can't depend on having btree_gist here,
--
2.34.1
