types: give REFCURSOR its own type family#111906
types: give REFCURSOR its own type family#111906craig[bot] merged 4 commits intocockroachdb:masterfrom
Conversation
|
This change is |
|
The first commit is #111884. |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 8 of 27 files at r2, 2 of 3 files at r3, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @DrewKimball, @mgartner, and @rharding6373)
pkg/sql/sem/tree/parse_string.go line 73 at r2 (raw file):
d, err = ParseDPGLSN(s) case types.RefCursorFamily: return NewDRefCursor(s), false, nil
nit: in this method we don't return from within the switch stmt, so I wouldn't do it for consistency here too.
1384c19 to
447656b
Compare
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/parse_string.go line 73 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: in this method we don't return from within the switch stmt, so I wouldn't do it for consistency here too.
Done.
|
From the tests it looks like there are a few places where the refcursor family is missing or built-in functions haven't been updated to return the new type. Pinging to bring this to your attention in case it isn't something you're looking into already. |
michae2
left a comment
There was a problem hiding this comment.
Reviewed 30 of 30 files at r6.
Reviewable status:
pkg/util/parquet/testutils.go line 366 at r6 (raw file):
// REFCURSOR is implemented using DOidWrapper. if wrapper.Oid != oid.T_refcursor { return unwrapDatum(wrapper.Wrapped)
Why is REFCURSOR implemented using DOidWrapper?
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/util/parquet/testutils.go line 366 at r6 (raw file):
Previously, michae2 (Michael Erickson) wrote…
Why is REFCURSOR implemented using DOidWrapper?
REFCURSOR behaves just like a string; the main difference is that it can't be freely casted the way string types can, and that behavior is encoded by the type rather than the underlying datum.
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/util/parquet/testutils.go line 366 at r6 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
REFCURSORbehaves just like a string; the main difference is that it can't be freely casted the way string types can, and that behavior is encoded by the type rather than the underlying datum.
DOidWrapper lets us share the implementation for the datum while also being able to specify type-specific behavior. Up until now it's only been used to change how a type's name is displayed, but it was intended to be usable for other behavio changes as well.
This patch removes the case statement that checks whether a cursor name is NULL before calling into `plpgsql_gen_cursor_name` because the builtin already checks whether the argument is NULL, and `REFCURSOR` will (correctly) not support comparisons after we add a new type family for it. The return type for the call to `plpgsql_gen_cursor_name` is also changed from `String` to `Refcursor`. Informs cockroachdb#111560 Release note: None
This patch adds the new `REFCURSOR` data type to the list of types that
a string constant can have. This will allow constant values to avoid
an explicit cast to `REFCURSOR` in cases when the context expects a
`REFCURSOR`, e.g. inserting into a `REFCURSOR` column or calling a
routine with a `REFCURSOR` parameter. Example:
```
CREATE FUNCTION f(curs REFCURSOR) RETURNS REFCURSOR AS $$
SELECT curs;
$$ LANGUAGE SQL;
-- This cast was necessary before:
SELECT f('foo'::REFCURSOR);
-- Now this works:
SELECT f('foo');
```
Informs cockroachdb#111560
Release note: None
447656b to
b8bb90b
Compare
|
I've moved the commit to allow parsing REFCURSOR constants before the one to add the type family; this decreases the number of test changes that had to be made between commits. I also added a commit to remove the check for NULL cursor name before calling |
Previously, REFCURSOR shared a type family with the string-like types. However, since it can't be implicitly casted to and from string types in all cases, it needs to have its own family (since types within a family must be compatible). This patch adds the new `RefCursorFamily`, and follows the example of the PG_LSN type in plumbing it throughout the SQL layer. Informs cockroachdb#111560 Release note: None
The `REFCURSOR` data type does not support comparisons apart from a few edge cases like `'foo'::REFCURSOR IS NULL`. Example: ``` postgres=# SELECT 'foo'::REFCURSOR = 'foo'::REFCURSOR; ERROR: operator does not exist: refcursor = refcursor ``` The previous commit that added the `RefCursorFamily` type family already effectively removed support for most comparisons; this commit also prevents the `IS / IS DISTINCT FROM` variants from being used on a `REFCURSOR` expression. Note that postgres supports a few cases with `NULL` as mentioned above. However, this case seems unimportant, so I leave it as a TODO for now. This commit also adds tests for various comparisons involving `REFCURSOR`. Informs cockroachdb#111560 Release note: None
b8bb90b to
e013f4b
Compare
michae2
left a comment
There was a problem hiding this comment.
(Don't forget to update the PR description to match the new commit order.)
Reviewed 38 of 38 files at r9, 35 of 35 files at r10, 33 of 37 files at r11, 3 of 5 files at r12, 3 of 3 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status:
pkg/util/parquet/testutils.go line 366 at r6 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
DOidWrapperlets us share the implementation for the datum while also being able to specify type-specific behavior. Up until now it's only been used to change how a type's name is displayed, but it was intended to be usable for other behavio changes as well.
Thanks!
|
TFTR! bors r+ |
|
Build succeeded: |
Previously, not unwrap datums read from parquet files before comparing. Now, we do. Previously, we didn't need to (all datums were unwrapped), but cockroachdb#111906 added a new decoder which may return an OID-wrapped datum. We do not need to preserve OID wrappers etc when reading parquet files in tests because we do not store that information in the parquet files. Testing that OID wrappers match before and after is unecessary. Additionally, we used to skip unwrapping `REFCURSOR` datums before comparing. For the same reason as above, we can just unwrap them. Release note: None Epic: None
Previously, we did not unwrap datums read from parquet files before comparing. Now, we do. Before cockroachdb#111906, we didn't need to, but now we do because that patch added a new decoder which may return an OID-wrapped datum. We do not need to preserve OID wrappers etc when reading parquet files in tests because we do not store that information in the parquet files. Testing that OID wrappers match before and after is unecessary. Additionally, we used to skip unwrapping `REFCURSOR` datums before comparing. For the same reason as above, we can just unwrap them. This change also fixes the `validateDatum` function in exporter tests. It previously did not unwrap array contents. Now it does. Release note: None Epic: None Closes: cockroachdb#112561 Please enter the commit message for your changes. Lines starting
112653: nightlies: add script for stress-engflow nightly r=rail a=rickystewart Epic: CRDB-8308 Release note: None 112687: release: verify version.txt in Pick SHA r=celiala a=rail Previously, we did not verify the contents of the version.txt file as a part of the Pick SHA step. In case the guessed version doesn't match the contents of the file, we have to delay the release by many hours to allow CI automation work. This PR adds a check in the Pick SHA step to verify the version. Fixes: RE-438 Release note: None Release justification: release automation changes 112689: streamclient: bump test size r=rail a=rickystewart This is routinely timing out in the EngFlow continuous test. Maybe when it's working on remote execution it will be quicker and we can bump this back down to `small`. Epic: CRDB-8308 Release note: None 112693: parquet: unwrap datums better r=jayshrivastava a=jayshrivastava Previously, we did not unwrap datums read from parquet files before comparing. Now, we do. Before #111906, we didn't need to, but now we do because that patch added a new decoder which may return an OID-wrapped datum. We do not need to preserve OID wrappers etc when reading parquet files in tests because we do not store that information in the parquet files. Testing that OID wrappers match before and after is unecessary. Additionally, we used to skip unwrapping `REFCURSOR` datums before comparing. For the same reason as above, we can just unwrap them. This change also fixes the `validateDatum` function in exporter tests. It previously did not unwrap array contents. Now it does. Release note: None Epic: None Closes: #112561 112704: zonepb: add expected/actual values to validation errors r=rafiss a=rafiss This will make it slightly easier to debug errors where this validation fails. informs #111299 Release note: None Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com> Co-authored-by: Rail Aliiev <rail@iqchoice.com> Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Previously, we did not unwrap datums read from parquet files before comparing. Now, we do. Before #111906, we didn't need to, but now we do because that patch added a new decoder which may return an OID-wrapped datum. We do not need to preserve OID wrappers etc when reading parquet files in tests because we do not store that information in the parquet files. Testing that OID wrappers match before and after is unecessary. Additionally, we used to skip unwrapping `REFCURSOR` datums before comparing. For the same reason as above, we can just unwrap them. This change also fixes the `validateDatum` function in exporter tests. It previously did not unwrap array contents. Now it does. Release note: None Epic: None Closes: #112561 Please enter the commit message for your changes. Lines starting
plpgsql: prepare cursor name generation for REFCURSOR type family
This patch removes the case statement that checks whether a cursor
name is NULL before calling into
plpgsql_gen_cursor_namebecause thebuiltin already checks whether the argument is NULL, and
REFCURSORwill (correctly) not support comparisons after we add a new type family
for it. The return type for the call to
plpgsql_gen_cursor_nameisalso changed from
StringtoRefcursor.Informs #111560
Release note: None
tree: add REFCURSOR to the list of parsable string types
This patch adds the new
REFCURSORdata type to the list of types thata string constant can have. This will allow constant values to avoid
an explicit cast to
REFCURSORin cases when the context expects aREFCURSOR, e.g. inserting into aREFCURSORcolumn or calling aroutine with a
REFCURSORparameter. Example:Informs #111560
Release note: None
types: give REFCURSOR its own type family
Previously, REFCURSOR shared a type family with the string-like types.
However, since it can't be implicitly casted to and from string types in
all cases, it needs to have its own family (since types within a family
must be compatible). This patch adds the new
RefCursorFamily, andfollows the example of the PG_LSN type in plumbing it throughout the
SQL layer.
Informs #111560
Release note: None
tree: disallow and test comparison operators for REFCURSOR
The
REFCURSORdata type does not support comparisons apart from afew edge cases like
'foo'::REFCURSOR IS NULL. Example:The previous commit that added the
RefCursorFamilytype family alreadyeffectively removed support for most comparisons; this commit also
prevents the
IS / IS DISTINCT FROMvariants from being used on aREFCURSORexpression. Note that postgres supports a few cases withNULLas mentioned above. However, this case seems unimportant, so Ileave it as a TODO for now.
This commit also adds tests for various comparisons involving
REFCURSOR.Informs #111560
Release note: None