Skip to content
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

DO blocks #19356

Merged
merged 5 commits into from
Feb 12, 2025
Merged

DO blocks #19356

merged 5 commits into from
Feb 12, 2025

Conversation

taroface
Copy link
Contributor

@taroface taroface commented Feb 6, 2025

DOC-10471

Note: This can't merge until a separate cockroach PR to add the SQL diagram is merged: cockroachdb/cockroach#140680

@taroface taroface requested a review from dikshant February 6, 2025 22:22
Copy link

github-actions bot commented Feb 6, 2025

Files changed:

Copy link

netlify bot commented Feb 6, 2025

Deploy Preview for cockroachdb-api-docs canceled.

Name Link
🔨 Latest commit 81c2dfd
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-api-docs/deploys/67acffae9e242100081bb903

Copy link

netlify bot commented Feb 6, 2025

Deploy Preview for cockroachdb-interactivetutorials-docs canceled.

Name Link
🔨 Latest commit 81c2dfd
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-interactivetutorials-docs/deploys/67acffae58655200089cffc7

Copy link

netlify bot commented Feb 6, 2025

Netlify Preview

Name Link
🔨 Latest commit 81c2dfd
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/67acffae393a690008caf387
😎 Deploy Preview https://deploy-preview-19356--cockroachdb-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@taroface taroface requested a review from DrewKimball February 6, 2025 22:30
Copy link

@dikshant dikshant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!


## Required privileges

- To execute a `DO` block, a user must have at least the [`USAGE` privilege]({% link {{ page.version.version }}/security-reference/authorization.md %}#supported-privileges) on the schema where the `DO` block is being executed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this one? I don't think it's true in either CRDB or Postgres.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sorry, that was kind of a guess. I ported it over from the CREATE PROCEDURE and CREATE FUNCTION prereqs. What exactly are the privileges we need on DO blocks? The PG doc says The user must have USAGE privilege for the procedural language, or must be a superuser if the language is untrusted. This is the same privilege requirement as for creating a function in the language.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't currently do language privileges AFAIK, so I think we should just leave that out for now.

{% remote_include https://raw.githubusercontent.com/cockroachdb/generated-diagrams/{{ page.release_info.crdb_branch_name }}/grammar_svg/do.html %}
</div>

## Parameters

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note there's also an optional LANGUAGE foo. SQL is disallowed and we currently only support PL/pgSQL out of the options Postgres allows.

Copy link
Contributor Author

@taroface taroface Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this will be in the diagram (will open that PR soon). But I didn't know about SQL being disallowed. So the diagram should only show an optional LANGUAGE PLPGSQL?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the diagram should only show an optional LANGUAGE PLPGSQL?

Yeah, that sounds good.

@taroface
Copy link
Contributor Author

taroface commented Feb 7, 2025

@DrewKimball Updated, PTAL.

Copy link

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@taroface taroface requested a review from florence-crl February 7, 2025 16:34
Copy link
Contributor

@florence-crl florence-crl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
just wondering: should there be a link on plpgsql.md to do.md? Such as:
"PL/pgSQL is a procedural language that you can use within user-defined functions, stored procedures, and DO blocks in CockroachDB.
"

@taroface
Copy link
Contributor Author

lgtm just wondering: should there be a link on plpgsql.md to do.md? Such as: "PL/pgSQL is a procedural language that you can use within user-defined functions, stored procedures, and DO blocks in CockroachDB. "

Great point -- I added this.

@taroface taroface enabled auto-merge (squash) February 10, 2025 16:48
@taroface taroface disabled auto-merge February 10, 2025 16:48
craig bot pushed a commit to cockroachdb/cockroach that referenced this pull request Feb 10, 2025
140680: docgen: add DO statement diagram r=DrewKimball a=taroface

Added a SQL statement diagram for `DO`. The diagram looks like this:

<img width="519" alt="image" src="https://github.com/user-attachments/assets/30feb56a-2085-4fa6-800c-d7920ec4fe49" />

This is necessary for the accompanying docs PR: cockroachdb/docs#19356

Epic: none
Release note: none
Release justification: non-production code change

Co-authored-by: Ryan Kuo <[email protected]>
@taroface taroface enabled auto-merge (squash) February 12, 2025 20:08
@taroface taroface merged commit 59fa4ff into main Feb 12, 2025
6 checks passed
@taroface taroface deleted the do-statement branch February 12, 2025 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants