On Wed, May 1, 2024 at 12:39 AM Paul Jungwirth
<p...@illuminatedcomputing.com> wrote:
>
> On 4/30/24 09:24, Robert Haas wrote:
> > Peter, could you have a look at
> > http://postgr.es/m/47550967-260b-4180-9791-b224859fe...@illuminatedcomputing.com
> > and express an opinion about whether each of those proposals are (a)
> > good or bad ideas and (b) whether they need to be fixed for the
> > current release?
>
> Here are the same patches but rebased. I've added a fourth which is my
> progress on adding the CHECK
> constraint. I don't really consider it finished though, because it has these
> problems:
>
> - The CHECK constraint should be marked as an internal dependency of the PK,
> so that you can't drop
> it, and it gets dropped when you drop the PK. I don't see a good way to tie
> the two together though,
> so I'd appreciate any advice there. They are separate AlterTableCmds, so how
> do I get the
> ObjectAddress of both constraints at the same time? I wanted to store the
> PK's ObjectAddress on the
> Constraint node, but since ObjectAddress isn't a Node it doesn't work.
>
> - The CHECK constraint should maybe be hidden when you say `\d foo`? Or maybe
> not, but that's what
> we do with FK triggers.
>
> - When you create partitions you get a warning about the constraint already
> existing, because it
> gets created via the PK and then also the partitioning code tries to copy it.
> Solving the first
> issue here should solve this nicely though.
>
> Alternately we could just fix the GROUP BY functional dependency code to only
> accept b-tree indexes.
> But I think the CHECK constraint approach is a better solution.
>
I will consider these issues later.
The following are general ideas after applying your patches.
CREATE TABLE temporal_rng1(
id int4range,
valid_at daterange,
CONSTRAINT temporal_rng1_pk unique (id, valid_at WITHOUT OVERLAPS)
);
insert into temporal_rng1(id, valid_at) values (int4range '[1,1]',
'empty'::daterange), ('[1,1]', 'empty');
table temporal_rng1;
id | valid_at
-------+----------
[1,2) | empty
[1,2) | empty
(2 rows)
i hope i didn't miss something:
exclude the 'empty' special value, WITHOUT OVERLAP constraint will be
unique and is more restrictive?
if so,
then adding a check constraint to make the WITHOUT OVERLAP not include
the special value 'empty'
is better than
writing a doc explaining that on some special occasion, a unique
constraint is not meant to be unique
?
in here
https://www.postgresql.org/docs/devel/ddl-constraints.html#DDL-CONSTRAINTS-UNIQUE-CONSTRAINTS
says:
<<
Unique constraints ensure that the data contained in a column, or a
group of columns, is unique among all the rows in the table.
<<
+ /*
+ * The WITHOUT OVERLAPS part (if any) must be
+ * a range or multirange type.
+ */
+ if (constraint->without_overlaps && lc == list_last_cell(constraint->keys))
+ {
+ Oid typid = InvalidOid;
+
+ 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;
+ }
+ }
+ }
+ else
+ typid = typenameTypeId(NULL, column->typeName);
+
+ if (OidIsValid(typid) && !type_is_range(typid) && !type_is_multirange(typid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("column \"%s\" in WITHOUT OVERLAPS is not a range or
multirange type", key),
+ parser_errposition(cxt->pstate, constraint->location)));
+ }
+ if (attr->attisdropped)
+ break;
it will break the loop?
but here you want to continue the loop?
+ if (OidIsValid(typid) && !type_is_range(typid) && !type_is_multirange(typid))
didn't consider the case where typid is InvalidOid,
maybe we can simplify to
+ if (!type_is_range(typid) && !type_is_multirange(typid))
+ notnullcmds = lappend(notnullcmds, notemptycmd);
seems weird.
we can imitate notnullcmds related logic for notemptycmd,
not associated notnullcmds in any way.
