Re: SQL:2011 application time - Mailing list pgsql-hackers

I committed a few fixes in this area today.  Has everything here been 
addressed?


On 16.08.24 04:12, jian he wrote:
> On Thu, Aug 8, 2024 at 4:54 AM Paul Jungwirth
> <pj@illuminatedcomputing.com> wrote:
>>
>> Rebased to e56ccc8e42.
> 
> I only applied to 0001-0003.
> in create_table.sgml, I saw the WITHOUT OVERLAPS change is mainly in
> table_constraint.
> but we didn't touch alter_table.sgml.
> Do we also need to change alter_table.sgml correspondingly?
> 
> 
> + if (constraint->without_overlaps)
> + {
> + /*
> + * This enforces that there is at least one equality column
> + * besides the WITHOUT OVERLAPS columns.  This is per SQL
> + * standard.  XXX Do we need this?
> + */
> + if (list_length(constraint->keys) < 2)
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("constraint using WITHOUT OVERLAPS needs at least two columns"));
> +
> + /* WITHOUT OVERLAPS requires a GiST index */
> + index->accessMethod = "gist";
> + }
> if Constraint->conname is not NULL, we can
> + errmsg("constraint \"%s\" using WITHOUT OVERLAPS needs at least two
> columns"));
> 
> "XXX Do we need this?"
> I think currently we need this, otherwise the following create_table
> synopsis will not be correct.
> UNIQUE [ NULLS [ NOT ] DISTINCT ] ( column_name [, ... ] [,
> column_name WITHOUT OVERLAPS ] )
> PRIMARY KEY ( column_name [, ... ] [, column_name WITHOUT OVERLAPS ] )
> 
> 
> we add a column in catalog-pg-constraint.
> do we need change column conexclop,
> "If an exclusion constraint, list of the per-column exclusion operators"
> but currently, primary key, unique constraint both have valid conexclop.
> 
> 
> +static void
> +ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum
> attval, char typtype, Oid atttypid)
> +{
> + bool isempty;
> + RangeType *r;
> + MultirangeType *mr;
> +
> + switch (typtype)
> + {
> + case TYPTYPE_RANGE:
> + r = DatumGetRangeTypeP(attval);
> + isempty = RangeIsEmpty(r);
> + break;
> + case TYPTYPE_MULTIRANGE:
> + mr = DatumGetMultirangeTypeP(attval);
> + isempty = MultirangeIsEmpty(mr);
> + break;
> + default:
> + elog(ERROR, "WITHOUT OVERLAPS column \"%s\" is not a range or multirange",
> + NameStr(attname));
> + }
> +
> + /* Report a CHECK_VIOLATION */
> + if (isempty)
> + ereport(ERROR,
> + (errcode(ERRCODE_CHECK_VIOLATION),
> + errmsg("empty WITHOUT OVERLAPS value found in column \"%s\" in
> relation \"%s\"",
> + NameStr(attname), RelationGetRelationName(rel))));
> +}
> I think in the default branch, you need at least set the isempty
> value, otherwise maybe there will be a compiler warning
> because later your use isempty, but via default branch is value undefined?
> 
> 
> + /*
> + * If this is a WITHOUT OVERLAPS constraint,
> + * we must also forbid empty ranges/multiranges.
> + * This must happen before we look for NULLs below,
> + * or a UNIQUE constraint could insert an empty
> + * range along with a NULL scalar part.
> + */
> + if (indexInfo->ii_WithoutOverlaps)
> + {
> +             ExecWithoutOverlapsNotEmpty(heap, att->attname,
> + }
> previously we found out that if this happens later, then it won't work.
> but this comment didn't explain why this must have happened earlier.
> I didn't dig deep enough to find out why.
> but explaining it would be very helpful.
> 
> 
> I think some tests are duplicated, so I did the refactoring.