sqlsmith: add support for DO block

[ZhouXing19]

informs #106368

A DO block can be either a top-level sql statement or be part of the plpgsql function body. In this commit we introduce the DO block for both of these 2 usages. We also introduced DisableDoBlocks which disables both usages of a DO block.

Release note: None

[cockroach-teamcity]

This change is 

[ZhouXing19]

I'm not sure where to put the new makeDoBlock in which set. For makeCreateFunc we now have

	altersFunctionExistence = []statementWeight{
		{10, makeCreateFunc},
		{1, makeDropFunc},
	}

but I don't know if i should simply append makeDoBlock into the same list.

[yuzefovich]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

---

-- commits line 2 at r1:
nit: we should update the commit message.

---

pkg/internal/sqlsmith/plpgsql.go line 144 at r1 (raw file):

		{2, makePLpgSQLIf},
		{2, makePLpgSQLWhile},
		{2, makeDoBlockWithinScope},

Given that DO block is only allowed as top-level SQL stmt, I don't think we should be including it into plpgsqlStmts. In other words, we should only be generating the DO block at the level of makeInsert or makeSelect.

We definitely want to somehow include it into allStatements in relational.go. Perhaps we need to have two variants like makeDoBlockNonMutating (which cannot execute any mutations) and general makeDoBlock (that can execute any stmts within).

---

pkg/internal/sqlsmith/relational.go line 921 at r1 (raw file):

func makeDoBlock(s *Smither) (tree.Statement, bool) {
	if s.disableUDFCreation {

nit: DO block seems orthogonal to UDF creation, so I don't think we should be checking it. We might want to provide a separate "disable DO block" sqlsmith option though, but I'd hold off on that until we find a test that needs it.

[ZhouXing19]

pkg/internal/sqlsmith/plpgsql.go line 144 at r1 (raw file):

Given that DO block is only allowed as top-level SQL stmt
hmm DO can actually be part of the function body, see example. But not sure if i misunderstood what you mean by top-level SQL stmt.

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @ZhouXing19)

---

pkg/internal/sqlsmith/plpgsql.go line 144 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Given that DO block is only allowed as top-level SQL stmt
hmm DO can actually be part of the function body, see example. But not sure if i misunderstood what you mean by top-level SQL stmt.

Oh, I was looking at tree.DoBlock from tree/do.go where I saw this comment. Make sense that it's also allowed to be within body of a routine, and we should try to generate both variants. (A couple of recent panics where the result of the DO block that wasn't inside a routine itself.)

[ZhouXing19]

pkg/internal/sqlsmith/plpgsql.go line 144 at r1 (raw file):

Perhaps we should consider two variants: makeDoBlockNonMutating (which disallows mutations) and a general makeDoBlock (which allows any statements).

However, looking more closely at how makePLpgSQLBlock() works—it calls makePLpgSQLExecSQL(), which in turn uses Smither.makeSQLStmtForRoutine() to generate the statements inside the DO block—it seems that we already enforce the Smither.disableMutations check there. In other words, a DO block automatically adapts its contents based on disableMutations.

Given that, I’m not sure it’s necessary to split into makeDoBlockNonMutating and makeDoBlock. Would it be acceptable to simply include makeDoBlock in both mutatingStatements and nonMutatingStatements? Or perhaps it deserves its own category (similar to how makeCreateFunc doesn’t fall neatly into either group)?

[yuzefovich]

Nice!

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @ZhouXing19)

---

pkg/internal/sqlsmith/plpgsql.go line 144 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Perhaps we should consider two variants: makeDoBlockNonMutating (which disallows mutations) and a general makeDoBlock (which allows any statements).

However, looking more closely at how makePLpgSQLBlock() works—it calls makePLpgSQLExecSQL(), which in turn uses Smither.makeSQLStmtForRoutine() to generate the statements inside the DO block—it seems that we already enforce the Smither.disableMutations check there. In other words, a DO block automatically adapts its contents based on disableMutations.

Given that, I’m not sure it’s necessary to split into makeDoBlockNonMutating and makeDoBlock. Would it be acceptable to simply include makeDoBlock in both mutatingStatements and nonMutatingStatements? Or perhaps it deserves its own category (similar to how makeCreateFunc doesn’t fall neatly into either group)?

Yeah, good point, I agree that existing disableMutations should work.

Including makeDoBlock into both mutating and non-mutating stmts sounds good to me - I'd only consider reducing the weights of the Do block, maybe to 1 in both cases, WDYT? The larger the weight, the more likely the stmt will be chosen, and we probably want Do blocks to be generated at roughly the same frequency as other "unusual" stmts like CREATE STATS. The fact that we're including Do blocks into both mutating and non-mutating sets will make Do blocks more frequent than "pure weight 1" stmts.

---

pkg/internal/sqlsmith/plpgsql.go line 413 at r2 (raw file):

		return nil, false
	}
	block, _ := makePLpgSQLBlock(s, scope)

nit: you could just use s.makePLpgSQLBlock(scope) to avoid having unnecessary ok return parameter.

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @yuzefovich)

---

pkg/internal/sqlsmith/plpgsql.go line 144 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Yeah, good point, I agree that existing disableMutations should work.

Including makeDoBlock into both mutating and non-mutating stmts sounds good to me - I'd only consider reducing the weights of the Do block, maybe to 1 in both cases, WDYT? The larger the weight, the more likely the stmt will be chosen, and we probably want Do blocks to be generated at roughly the same frequency as other "unusual" stmts like CREATE STATS. The fact that we're including Do blocks into both mutating and non-mutating sets will make Do blocks more frequent than "pure weight 1" stmts.

Good idea, reduced both weights to 1.