Re: SQL:2011 application time - Mailing list pgsql-hackers
From | Paul Jungwirth |
---|---|
Subject | Re: SQL:2011 application time |
Date | |
Msg-id | 2d9740ad-3bb7-473c-8441-344351caa8ee@illuminatedcomputing.com Whole thread Raw |
In response to | Re: SQL:2011 application time (jian he <jian.universality@gmail.com>) |
Responses |
Re: SQL:2011 application time
Re: SQL:2011 application time |
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: