Re: SQL:2011 application time

From: Paul Jungwirth <pj(at)illuminatedcomputing(dot)com>
To: jian he <jian(dot)universality(at)gmail(dot)com>
Cc: Isaac Morland <isaac(dot)morland(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Jeff Davis <pgsql(at)j-davis(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: SQL:2011 application time
Date: 2024-08-07 20:54:52
Message-ID: 081aa43c-3a97-4bd5-a498-ba02e0a7570d@illuminatedcomputing.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v39-0001-Add-stratnum-GiST-support-function.patch text/x-patch 20.7 KB
v39-0002-Add-temporal-PRIMARY-KEY-and-UNIQUE-constraints.patch text/x-patch 148.1 KB
v39-0003-Forbid-empty-ranges-multiranges-in-WITHOUT-OVERL.patch text/x-patch 29.4 KB
v39-0004-Add-temporal-FOREIGN-KEY-contraints.patch text/x-patch 186.9 KB
v39-0005-Add-support-funcs-for-FOR-PORTION-OF.patch text/x-patch 43.3 KB
v39-0006-Add-UPDATE-DELETE-FOR-PORTION-OF.patch text/x-patch 160.3 KB
v39-0007-Add-CASCADE-SET-NULL-SET-DEFAULT-for-temporal-fo.patch text/x-patch 225.1 KB
v39-0008-Add-PERIODs.patch text/x-patch 334.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Sami Imseih 2024-08-07 21:22:22 Re: Restart pg_usleep when interrupted
Previous Message Thomas Munro 2024-08-07 20:52:41 Re: Remaining dependency on setlocale()