-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crosscluster/logical: set UDF function name from SQL input #127428
Conversation
5786905
to
3d366ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @azhu-crl, @dt, and @stevendanna)
pkg/ccl/crosscluster/logical/logical_replication_job.go
line 379 at r5 (raw file):
// processors. This makes it a fragile to // renames. fnName = fmt.Sprintf("%s.%s",
we could pass it by OID instead. in case you weren't aware, we already have this syntax:
root@localhost:26257/defaultdb> CREATE FUNCTION add(a INT, b INT) RETURNS INT LANGUAGE SQL AS $$ SELECT a + b; $$;
CREATE FUNCTION
Time: 28ms total (execution 20ms / network 8ms)
root@localhost:26257/defaultdb> select 'add'::regproc::oid;
oid
----------
100105
(1 row)
Time: 2ms total (execution 2ms / network 1ms)
root@localhost:26257/defaultdb> SELECT [FUNCTION 100105](1, 2);
add
-------
3
(1 row)
Oh nice, I like that via-OID call that sidesteps all resolution messiness. |
Oh cool, it works across databases as well. Very useful! |
This wires up the function from the SQL syntax to the processor. Epic: none Release note: None
Most practically, this feature is what required the extra type which is a bit annoying. Further, as shown in one of the updated tests, the user can insert to the table manually if they want. Release note: None Epic: none
Epic: none Release note: None
76c00c2
to
4a37a07
Compare
Well that cleans things up rather nicely. |
4a37a07
to
e8e5a37
Compare
Changes lookgood to me, but will defer to Rafi with more context on the UDF work. I'll wait for these changes to then rebase create_logical_replication_stmt onto my open PR now. |
729f7b4
to
5963d57
Compare
This cleans up a lot of the work we were doing just to get the database name so that we could call the function by name. Epic: none Release note: None
5963d57
to
95f9c81
Compare
bors r=dt |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 46d61f8 to blathers/backport-release-24.2-127428: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
@stevendanna i believe the backport for this needs to merge after #127538 |
First commit is #127024
See individual commits.