Re: SQL:2011 application time - Mailing list 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@illuminatedcomputing.com