Re: SQL:2011 application time

Hello,

Here is a new set of patches, including new patches to (1) fix logical replication with WITHOUT
OVERLAPS indexes and (2) address some documentation lapses pointed out in jian he's feedback. Since
all that is against the already-commited PK/UNIQUE/FK work, I've kept them separate here from the
FOR PORTION OF etc patches. I've also added the logical replication problem to the v18 Open Items
wiki page.

Details below:

On 8/15/24 19:12, jian he wrote:
> 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?

Good catch! Not just WITHOUT OVERLAPS but FOREIGN KEY needed to be updated too. Patched here.

> + 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 ] )

Having zero scalar columns is not allowed according to SQL:2011, but supporting it seems like a cool
thing to do eventually. It doesn't have to be in this first set of patches. But if we support it
someday, I agree we must update these docs to match.

> 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.

I updated the docs to include the WITHOUT OVERLAPS case.

> +static void
> +ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum
> attval, char typtype, Oid atttypid)
> ...
> 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?

This isn't getting a warning on the build farm, so I think compilers are smart enough to see that
elog exits.

> + /*
> + * 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 don't follow. Explaining why it can't happen later is the same as explaining why it must happen
sooner, right? But you say "must *have* happened earlier"---it's not that it must have happened
already, but that we must do it now (before bailing when we look for NULLs).

> I think some tests are duplicated, so I did the refactoring.

I kept some of this patch: querying tableoid::regclass from the partition root is nicer than
querying the leaves separately.

The changes removing DELETEs & INSERTs made the tests less local, since they would depend on inserts
made further up in the file. I'd rather keep the tests fairly isolated. I don't think this is the
same thing as duplication.

Rebased to 68ad9816c1.

Yours,

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com