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.universality@gmail.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.universality@gmail.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.universality@gmail.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.universality@gmail.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@illuminatedcomputing.com