Re: SQL:2011 application time

Thanks for all the feedback! Consolidating several emails below:

> On Fri, Oct 20, 2023 at 5:45 AM jian he <jian(dot)universality(at)gmail(dot)com>
wrote:
> I don't think we need to check attr->attisdropped here

Changed.

> "return *typnamespace;" should be "return true"?

No, but I added a comment to clarify.

> Maybe name it get_typname_and_typnamespace?

I could go either way on this but I left it as-is since it seems
redundant, and there are other functions here that don't repeat the
three-letter prefix.

> you can just `elog(ERROR, "missing range type %s", range_type_name);` ?

No, because this failure happens trying to look up the name.

> Also, this should be placed just below if
(!type_is_range(attr->atttypid))?

We ereport there (not elog) because it's a user error (using a
non-rangetype for the option), not an internal error.

> periods are always associated with the table, is the above else
branch correct?

True but I'm following the code just above for OCLASS_CONSTRAINT. Even
if this case is unexpected, it seems better to handle it gracefully than
have a harder failure.

> XXX: Does this hold for periods?
> Yes. we can add the following 2 sql for code coverage.
> alter table pt add period for tableoid (ds, de);
> alter table pt add period for "........pg.dropped.4........" (ds, de);

Added, thanks!

> On Sun, Oct 22, 2023 at 5:01 PM jian he <jian(dot)universality(at)gmail(dot)com>
wrote:
> drop table if exists for_portion_of_test1;
> CREATE unlogged TABLE for_portion_of_test1 (id int4range, valid_at
> tsrange,name text );
> ...

These are good tests, thanks! Originally FOR PORTION OF required a
PRIMARY KEY or UNIQUE constraint, so we couldn't find NULLs here, but we
changed that a while back, so it's good to verify it handles that case.

> 1279: if (isNull)
> 1280: elog(ERROR, "found a NULL range in a temporal table");
> 1281: oldRangeType = DatumGetRangeTypeP(oldRange);
>
> I wonder when this isNull will be invoked. The above tests won't
> invoke the error.

As far as I can tell it shouldn't happen, which is why it's elog. The
new tests don't hit it because a NULL range should never match the range
in the FROM+TO of the FOR PORTION OF clause. Maybe this should even be
an assert, but I think I prefer elog for the nicer error message and
less-local condition.

> also the above test, NULL seems equivalent to unbounded. FOR PORTION
> OF "from and "to" both bound should not be null?

Correct, NULL and UNBOUNDED mean the same thing. This matches the
meaning of NULL in ranges.

> which means the following code does not work as intended? I also
> cannot find a way to invoke the following elog error branch.
> File:src/backend/executor/nodeModifyTable.c
> 4458: exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange,
estate);
> 4459: targetRange = ExecEvalExpr(exprState, econtext, &isNull);
> 4460: if (isNull)
> 4461: elog(ERROR, "Got a NULL FOR PORTION OF target range");

Here we're checking the "target range", in other words the range built
from the FROM+TO of the FOR PORTION OF clause---not a range from a
tuple. Finding a NULL here *for the range itself* would indeed be an
error. A NULL *bound* means "unbounded", but a NULL *range* should not
be possible to construct.

> I also made some changes in the function range_leftover_internal,

I'm not really comfortable with these changes. "Leftover" doesn't refer
to "left" vs "right" but to what *remains* (what is "left behind") after
the UPDATE/DELETE. Also r1 and r2 are common parameter names throughout
the rangetypes.c file, and they are more general than the names you've
suggested. We shouldn't assume we will only ever call this function from
the FOR PORTION OF context.

> ExecForPortionOfLeftovers

Thanks! I've made these code changes (with slight modifications, e.g. no
need to call ExecFetchSlotHeapTuple if there are no leftovers).

I'm not sure about the comment change though---I want to verify that
myself (particularly the case when the partition key is updated so we
have already been routed to a different partition than the old tuple).

> On Tue, Oct 24, 2023 at 11:14 PM jian he
<jian(dot)universality(at)gmail(dot)com> wrote:
> I refactored findNewOrOldColumn to better handle error reports.

Thanks, I like your changes here. Applied with some small adjustments.

> On Sat, Oct 28, 2023 at 1:26 AM jian he <jian(dot)universality(at)gmail(dot)com>
wrote:
> I think it means, if the foreign key has PERIOD column[s], then the
> PERIOD column[s] will not be set to NULL in {ON DELETE|ON UPDATE}. . . .

I reworded this to explain that the PERIOD element will not be set to
NULL (or the default value).

> can you add the following for the sake of code coverage. I think
> src/test/regress/sql/without_overlaps.sql can be simplified.
> ...
> call overlaps_template();

I'm not sure I want to add indirection like this to the tests, which I
think makes them harder to read (and update). But there is indeed a
tough combinatorial explosion, especially in the foreign key tests. We
want to cover {ON DELETE,ON UPDATE} {NO ACTION,RESTRICT,CASCADE,SET
NULL,SET DEFAULT} when {child inserts,child updates,parent
updates,parent deletes} with {one,two} scalar columns and {,not}
partitioned. Also ON DELETE SET {NULL,DEFAULT} against only a subset of
columns. I updated the test cases to delete and re-use the same id
values, so at least they are more isolated and thus easier to edit. I
also added tests for `(parent_id1, parent2, PERIOD valid_at)` cases as
well as `ON DELETE SET {NULL,DEFAULT} (parent_id1)`. (I think that last
case covers what you are trying to do here, but if I misunderstood
please let me know.)

I haven't worked through your last email yet, but this seemed like
enough changes to warrant an update.

New patches attached (rebased to 0bc726d9).

Yours,

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