On Fri, Aug 2, 2024 at 1:09 AM Paul Jungwirth
<p...@illuminatedcomputing.com> wrote:
>
> On 7/25/24 08:52, Paul Jungwirth wrote:
> > Here is a patch moving the not-empty check into
> > check_exclusion_or_unique_constraint. That is a more
> > logical place for it than ExecConstraints, since WITHOUT OVERLAPS is part
> > of the index constraint
> > (not a CHECK constraint). At that point we've already looked up all the
> > information we need. So
> > there is no extra cost for non-temporal tables, and no need to change
> > pg_class or add to the
> > relcache. Also putting it there means we don't need any extra code to
> > enforce non-empties when we
> > build the index or do anything else with it.
> >
> > I think this is the nicest solution we can expect. It is even cleaner than
> > the &&& ideas. So
> > hopefully this gets us back to where we were when we decided to commit PKs
> > & FKs to v17.
> >
> > As before, I've left the nonempty check as a separate patch to make
> > reviewing easier, but when
> > committing I would squash it with the PK patch.
>
> Hello,
>
> Here is an updated set of patches, rebased because the old patches no longer
> applied.
>
void
ExecWithoutOverlapsNotEmpty(Relation rel, Datum attval, Oid typtype,
Oid atttypid);
should this just be a static function?
I am not so sure.
Oid typtype
should be
char typtype
?
errmsg("new row for relation \"%s\" contains empty
WITHOUT OVERLAPS value",
we already have Form_pg_attribute via "TupleDesc tupdesc =
RelationGetDescr(heap);"
we can make the error message be:
errmsg("cannot be empty range value for WITHOUT
OVERLAPS column \"%s\" in relation \"%s\", colname,
RelationGetRelationName(rel))
elog(ERROR, "Got unknown type for WITHOUT OVERLAPS column: %d", atttypid);
people will wonder if domain over range works or not. but currently
not, better error message would be:
elog(ERROR, "WITHOUT OVERLAPS column \"%s\" is not a range
or multirange type ", colname);
This part is unlikely to be reachable, so I don't have a strong opinion on it.
+ if (!found)
+ column = NULL;
this part no need?
because if not found, the column would be last element in ColumnDef
type list columns
also the following change also make sense:
+ if (!OidIsValid(typid) && column)
+ typid = typenameTypeId(NULL, column->typeName);
+ /* The WITHOUT OVERLAPS part (if any) must be a range or multirange type. */
+ if (constraint->without_overlaps && lc == list_last_cell(constraint->keys))
+ {
+ if (!found && cxt->isalter)
+ {
+ /*
+ * Look up the column type on existing table.
+ * If we can't find it, let things fail in DefineIndex.
+ */
+ Relation rel = cxt->rel;
+ for (int i = 0; i < rel->rd_att->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(rel->rd_att, i);
+ const char *attname;
+
+ if (attr->attisdropped)
+ break;
+
+ attname = NameStr(attr->attname);
+ if (strcmp(attname, key) == 0)
+ {
+ typid = attr->atttypid;
+ break;
+ }
+ }
+ }
+ if (found)
+{
+}
I am confused with this change?
you found out the typid,but didn't using this information, should it be
+ if (strcmp(attname, key) == 0)
+ {
+ typid = attr->atttypid;
+ found = true;
+ break;
+ }
so the failing error message be same for the following two cases:
CREATE TABLE t1 (id int4range,valid_at tsrange,b text,
CONSTRAINT temporal_rng_pk PRIMARY KEY (id, b WITHOUT OVERLAPS)
);
CREATE TABLE t1 (id int4range,valid_at tsrange,b text);
alter table t1 add CONSTRAINT temporal_rng_pk PRIMARY KEY (id, b
WITHOUT OVERLAPS);
