Re: SQL:2011 application time

Here are some fixes based on outstanding feedback (some old some new). Details below:

On 3/25/24 17:00, jian he wrote:
> hi.
> minor issues I found in v33-0003.
> there are 29 of {check_amproc_signature?.*false}
> only one {check_amproc_signature(procform->amproc, opcintype, true}
> is this refactoring really worth it?

I could add a separate function, for example check_amproc_retset_signature, but it would require
duplicating almost the whole existing function, so a param seems better here.

> We also need to refactor gistadjustmembers?

You're right, added the new support procs there.

> + <row>
> + <entry><function>intersect</function></entry>
> + <entry>computes intersection with <literal>FOR PORTION OF</literal>
> + bounds</entry>
> + <entry>13</entry>
> + </row>
> + <row>
> + <entry><function>without_portion</function></entry>
> + <entry>computes remaining duration(s) outside
> + <literal>FOR PORTION OF</literal> bounds</entry>
> + <entry>14</entry>
> + </row>
> needs to add "(optional)".

Added.

> +<programlisting>
> +Datum
> +my_range_intersect(PG_FUNCTION_ARGS)
> +{
> + RangeType *r1 = PG_GETARG_RANGE_P(0);
> + RangeType *r2 = PG_GETARG_RANGE_P(1);
> + TypeCacheEntry *typcache;
> +
> + /* Different types should be prevented by ANYRANGE matching rules */
> + if (RangeTypeGetOid(r1) != RangeTypeGetOid(r2))
> elog(ERROR, "range
> types do not match");
> +
> + typcache = range_get_typcache(fcinfo, RangeTypeGetOid(r1));
> +
> + PG_RETURN_RANGE_P(range_intersect_internal(typcache, r1, r2));
> +}
> +</programlisting>
> the elog, ERROR indentation is wrong?

Fixed.

> +/*
> + * range_without_portion_internal - Sets outputs and outputn to the ranges
> + * remaining and their count (respectively) after subtracting r2 from r1.
> + * The array should never contain empty ranges.
> + * The outputs will be ordered. We expect that outputs is an array of
> + * RangeType pointers, already allocated with two slots.
> + */
> +void
> +range_without_portion_internal(TypeCacheEntry *typcache, RangeType *r1,
> + RangeType *r2, RangeType **outputs, int *outputn)
> the comments need to be refactored?
> there is nothing related to "slot"?
> not sure the "array" description is right.
> (my understanding is compute rangetype r1 and r2, and save the result to
> RangeType **outputs.

Changed "slots" to "elements". Everything else looks correct to me.

> select proisstrict, proname from pg_proc where proname =
> 'range_without_portion';
> range_without_portion is strict.
> but
> select range_without_portion(NULL::int4range, int4range(11, 20,'[]'));
> return zero rows.
> Is this the expected behavior?

Returning zero rows is correct if the function is never called (which is what strict does).
I see other strict retset functions, e.g. json_array_elements.
That also returns zero rows if you say SELECT json_array_elements(NULL);

On 4/14/24 17:00, jian he wrote:
> for unique index, primary key:
> ii_ExclusionOps, ii_UniqueOps is enough to distinguish this index
> support without overlaps,
> we don't need another ii_HasWithoutOverlaps?
> (i didn't test it though)

I think it is worth having something named. But also ii_Exclusion is not set in
index_concurrently_create_copy, so inferring when we have WITHOUT OVERLAPS will not work in that case.

> ON CONFLICT DO NOTHING
> ON CONFLICT (id, valid_at) DO NOTHING
> ON CONFLICT ON CONSTRAINT temporal_rng_pk DO NOTHING
> I am confused by the test.
> here temporal_rng only has one primary key, ON CONFLICT only deals with it.
> I thought these three are the same thing?

They all have somewhat different code paths in infer_arbiter_indexes, and they mean different
things. I recall when I first started dealing with empty ranges several of these test cases caught
different bugs (as well as the DO UPDATE cases).

On 8/5/24 19:02, jian he wrote:
> void
> ExecWithoutOverlapsNotEmpty(Relation rel, Datum attval, Oid typtype,
> Oid atttypid);
>
> should this just be a static function?
> I am not so sure.

Changed. In a previous version I was calling this from two places, but I'm not anymore.

> Oid typtype
> should be
> char typtype
> ?

Oops, you're right! Fixed.

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

Yes, it's nicer to report the column name. Changed.

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

Likewise.

> + if (!found)
> + column = NULL;
> this part no need?
> because if not found, the column would be last element in ColumnDef
> type list columns

We can later set `found` to true from inheritance (or it being a system column), and then `column`
is set but wrong. So setting `column` to null seems generally clearer. But concretely, I use
`column` below to give me the type (which I otherwise don't have in CREATE TABLE), so I can forbid
types other than range and multirange.

> also the following change also make sense:
>
> + if (!OidIsValid(typid) && column)
> + typid = typenameTypeId(NULL, column->typeName);

This is because in CREATE TABLE I need to get the type from the `column` variable.

> 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;
> + }

Yes. Actually that is in the PERIOD patch file, but it should be in Forbid-empty-ranges. Moved.

> 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);

I think the same error message is the right thing to do here.
It looks like that's what we're doing.
If I've misunderstand what you want, can you clarify?

On 8/6/24 07:50, jian he wrote:
> in generateClonedIndexStmt
> index->iswithoutoverlaps = (idxrec->indisprimary ||
> idxrec->indisunique) && idxrec->indisexclusion;
> this case, the index accessMethod will be "gist" only?
>
> do you think it's necessary to:
> index->iswithoutoverlaps = (idxrec->indisprimary ||
> idxrec->indisunique) && idxrec->indisexclusion
> && strcmp(index->accessMethod, "gist") == 0);

This doesn't seem necessary, and maybe we'll support non-gist someday, when this condition would be
misleading.

> src/bin/pg_dump/pg_dump.c and src/bin/psql/describe.c
> should be "if (pset.sversion >= 180000)"?

Ah, thanks. Changing these from 170000 also landed in the wrong patch file. Fixed.

> + (This is sometimes called a
> + temporal key, if the column is a range of dates or timestamps, but
> + PostgreSQL allows ranges over any base type.)
>
> PostgreSQL should be decorated as
> <productname>PostgreSQL</productname>

Done.

> in DefineIndex we have:
> if (stmt->unique && !stmt->iswithoutoverlaps && !amRoutine->amcanunique)
> if (stmt->indexIncludingParams != NIL && !amRoutine->amcaninclude)
> if (numberOfKeyAttributes > 1 && !amRoutine->amcanmulticol)
> if (exclusion && amRoutine->amgettuple == NULL)
>
> maybe we can add:
> if (stmt->iswithoutoverlaps && strcmp(accessMethodName, "gist") != 0)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("access method \"%s\" does not support WITHOUT
> OVERLAPS constraints",
> accessMethodName)));

Okay.

> + /* exclusionOpNames can be non-NIL if we are creating a partition */
> + if (iswithoutoverlaps && exclusionOpNames == NIL)
> + {
> + indexInfo->ii_ExclusionOps = palloc_array(Oid, nkeycols);
> + indexInfo->ii_ExclusionProcs = palloc_array(Oid, nkeycols);
> + indexInfo->ii_ExclusionStrats = palloc_array(uint16, nkeycols);
> + }
> the comment is not 100% correct, i think.
> creating a partition, "create table like INCLUDING ALL", both will go
> through generateClonedIndexStmt.
> generateClonedIndexStmt will produce exclusionOpNames if this index
> supports exclusion constraint.

I think the comment is correct, but non-NIL is a confusing double negative, and it's not clear that
the comment is giving the motivation for the second half of the condition.
I re-wrote it to be more clear. I also adjusted the `if` to avoid parsing operator names when not
needed.

Rebased to e56ccc8e42.

Yours,

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