sql: support sequence and udt name rewriting in plpgsql #115809

rharding6373 · 2023-12-07T19:32:48Z

CRDB rewrites sequence and UDT names as IDs in views and functions so
that if the sequence or UDT is renamed the views and functions using
them don't break. This PR adds support for this in PLpgSQL.

Epic: None
Fixes: #115627

Release note: Fixes a bug in PLpgSQL where altering the name of a
sequence or UDT that was used in a PLpgSQL function or procedure could
break them. This is only present in 23.2 alpha and beta releases.

blathers-crl · 2023-12-07T19:32:52Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-12-07T19:32:58Z

This change is

rharding6373

First PR is #115144

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

rharding6373

Still needs the following: 1) Replace the IDs with names for SHOW CREATE, 2) UDT rewrite support.

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

DrewKimball

Reviewed 26 of 26 files at r1, 7 of 10 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rharding6373)

pkg/sql/sem/plpgsqltree/exception.go line 50 at r2 (raw file):

	newStmt, _ = visitor.Visit(s)
	for _, stmt := range s.Action {
		// TODO

norm.Factory.replaceFiltersExpr has a nice pattern for replacing a slice where one or more elements could change (or none).

pkg/sql/sem/plpgsqltree/statements.go line 1039 at r2 (raw file):

func (s *DynamicExecute) WalkStmt(visitor StatementVisitor) (newStmt Statement, changed bool) {
	newStmt, changed = visitor.Visit(s)

I think it'd be fine to just have this method panic for statements that haven't been fully implemented yet.

panic(unimplemented.New(  
"unimplemented PL/pgSQL statement",  
"attempted to use a PL/pgSQL statement that is not yet supported",  
))

Same elsewhere. That should decrease the surface area you have to worry about.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 120 at r2 (raw file):

type SQLStmtVisitor struct {

This is great!

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 159 at r2 (raw file):

		}
	case *plpgsqltree.Open:
		s, v.Err = tree.SimpleStmtVisit(t.Query, v.Fn)

Query can be nil here. Is that a problem for SimpleStmtVisit?

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 170 at r2 (raw file):

		}
	case *plpgsqltree.Declaration:
		e, v.Err = tree.SimpleVisit(t.Expr, v.Fn)

Expr can be nil.

Also, note: field Typ will have to be replaced when we add support for udt rewriting. I don't think there are any other cases like that.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 309 at r2 (raw file):

		}
	case *plpgsqltree.Exit:
		e, v.Err = tree.SimpleVisit(t.Condition, v.Fn)

Condition can be nil here and below.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 331 at r2 (raw file):

		}
	case *plpgsqltree.Return:
		e, v.Err = tree.SimpleVisit(t.Expr, v.Fn)

It isn't the case now, but in the future it will be possible to use a RETURN without a parameter.

rharding6373

This is ready for a closer review -- it now supports rewriting both sequences and UDTs round trip, and I polished up the code a bit more.

Apologies for the big PR. Let me know if you'd prefer me to break it up.

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

pkg/sql/sem/plpgsqltree/exception.go line 50 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

norm.Factory.replaceFiltersExpr has a nice pattern for replacing a slice where one or more elements could change (or none).

I was trying to avoid modifying the AST in place and opting to make copies of the nodes instead, because of #22847. I made the slice copies in CopyNode, echoing the pattern used in pkg/sql/sem/tree/walk.go. Let me know how you think this compares to your suggestion.

pkg/sql/sem/plpgsqltree/statements.go line 1039 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I think it'd be fine to just have this method panic for statements that haven't been fully implemented yet.
panic(unimplemented.New(  
"unimplemented PL/pgSQL statement",  
"attempted to use a PL/pgSQL statement that is not yet supported",  
))
Same elsewhere. That should decrease the surface area you have to worry about.

As a reviewer, do you have a preference? I'm on the fence, because on the one hand I implemented them all already and I'm also worried about supporting rewriting when adding support for these features because how are we going to remember? (The latter is more of a concern for taking unsupported nodes out of SQLStmtVisitor.Visit than WalkStmt). But on the other hand, YAGNI and there's no way to test these, so they're probably buggy anyway.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 120 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

This is great!

Thanks :-)

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 159 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Query can be nil here. Is that a problem for SimpleStmtVisit?

I added a nil check to the visitor function to handle this. I added a comment to the SQLStmtVisitor about this, to try to avoid running into this bug in the future.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 170 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Expr can be nil.

Also, note: field Typ will have to be replaced when we add support for udt rewriting. I don't think there are any other cases like that.

Nice catch on the Typ field.

For nil Expr, I have the visitor function handle it.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 309 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Condition can be nil here and below.

See above

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 331 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

It isn't the case now, but in the future it will be possible to use a RETURN without a parameter.

I think this should be resilient to that addition.

DrewKimball

This looks good!

Reviewed 1 of 10 files at r2, 29 of 34 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rharding6373)

pkg/sql/sem/plpgsqltree/exception.go line 50 at r2 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

I was trying to avoid modifying the AST in place and opting to make copies of the nodes instead, because of #22847. I made the slice copies in CopyNode, echoing the pattern used in pkg/sql/sem/tree/walk.go. Let me know how you think this compares to your suggestion.

The approach looks good!

pkg/sql/sem/plpgsqltree/exception.go line 60 at r4 (raw file):

		if ch {
			changed = true
			if newStmt != stmt {

This should be newStmt == s instead, right?

pkg/sql/sem/plpgsqltree/statements.go line 1039 at r2 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

As a reviewer, do you have a preference? I'm on the fence, because on the one hand I implemented them all already and I'm also worried about supporting rewriting when adding support for these features because how are we going to remember? (The latter is more of a concern for taking unsupported nodes out of SQLStmtVisitor.Visit than WalkStmt). But on the other hand, YAGNI and there's no way to test these, so they're probably buggy anyway.

You could make the point that implementing these as support for each statement is added will make sure we remember to test every case, but I'm fine with keeping them since you already did the work. If we call any of these methods it would be a bug right now anyway.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 124 at r4 (raw file):

type SQLStmtVisitor struct {
	Fn    tree.SimpleVisitFn
	TypFn func(typ tree.ResolvableTypeReference) (newTyp tree.ResolvableTypeReference, err error)

[nit] Couldn't we just make a separate visitor for this that only changes Declaration? Maybe like this:

// TypeRefVisitor calls the given replace function on each type reference contained in the visited PLpgSQL
// statements. Note that this currently only includes `Declaration`, and SQL statements and expressions
// are not visited.
type TypeRefVisitor struct {
    Fn func(typ tree.ResolvableTypeReference) (newTyp tree.ResolvableTypeReference, err error)
    Err error
}

var _ plpgsqltree.StatementVisitor = &TypeRefVisitor{}

func (v *TypeRefVisitor) Visit(stmt plpgsqltree.Statement) (newStmt plpgsqltree.Statement, changed bool) {
    if v.Err != nil {
        return stmt, false
    }
    if dec, ok := stmt.(*plpgsqltree.Declaration); ok {
        var newTyp tree.ResolvableTypeReference
        newTyp, v.Err = v.TypFn(dec.Typ)
        if v.Err != nil {
            return stmt, false
        }
        if  dec.Typ != newTyp {
            newStmt := dec.CopyNode()
            newStmt.(*plpgsqltree.Declaration).Typ = newTyp
            return newStmt, true
        }
    }
    return stmt, false
}

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 60 at r4 (raw file):

    END LOOP;
    OPEN curs FOR SELECT nextval('seq');
    RETURN nextval('seq');

Could you add an exception block here and below?

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 133 at r4 (raw file):

SELECT get_body_str('f_rewrite');
----
"DECLARE\nday @100107 := b'\\x80':::@100107;\ncurs REFCURSOR := b' ':::@100107::STRING;\ncurs2 CURSOR FOR SELECT b'@':::@100107;\nBEGIN\nRAISE notice\nUSING MESSAGE = format('val: %d':::STRING, b'\\x80':::@100107);\nRAISE notice 'val1: %, val2: %', b'\\x80':::@100107, b'\\xa0':::@100107;\nWHILE day != b'\\x80':::@100107 LOOP\nday := b'\\xc0':::@100107;\nSELECT b'\\x80':::@100107;\nIF day = b'\\x80':::@100107 THEN\n\tCONTINUE;\nELSIF day = b' ':::@100107 THEN\n\tSELECT b'@':::@100107 INTO day;\nEND IF;\nEND LOOP;\nOPEN curs FOR SELECT b'\\x80':::@100107;\nRETURN b'\\x80':::@100107;\nEND\n;"

I doubt it's because of this PR, but do you know why the casts were changed to type annotations?

rharding6373

Thanks for taking a look! RFAL

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

pkg/sql/sem/plpgsqltree/exception.go line 50 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

The approach looks good!

Done.

pkg/sql/sem/plpgsqltree/exception.go line 60 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

This should be newStmt == s instead, right?

Done.

pkg/sql/sem/plpgsqltree/statements.go line 1039 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

You could make the point that implementing these as support for each statement is added will make sure we remember to test every case, but I'm fine with keeping them since you already did the work. If we call any of these methods it would be a bug right now anyway.

After thinking about this over the weekend, I decided to go with your suggestion and add unimplemented panics both here and in the visitor. They should trigger when someone adding these features adds their first logic test, so that should be enough to make sure they add both implementation and test coverage for these. Hopefully this is easier to review with ~half as much new code. Thanks for the suggestion!

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 124 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] Couldn't we just make a separate visitor for this that only changes Declaration? Maybe like this:

// TypeRefVisitor calls the given replace function on each type reference contained in the visited PLpgSQL
// statements. Note that this currently only includes `Declaration`, and SQL statements and expressions
// are not visited.
type TypeRefVisitor struct {
    Fn func(typ tree.ResolvableTypeReference) (newTyp tree.ResolvableTypeReference, err error)
    Err error
}

var _ plpgsqltree.StatementVisitor = &TypeRefVisitor{}

func (v *TypeRefVisitor) Visit(stmt plpgsqltree.Statement) (newStmt plpgsqltree.Statement, changed bool) {
    if v.Err != nil {
        return stmt, false
    }
    if dec, ok := stmt.(*plpgsqltree.Declaration); ok {
        var newTyp tree.ResolvableTypeReference
        newTyp, v.Err = v.TypFn(dec.Typ)
        if v.Err != nil {
            return stmt, false
        }
        if  dec.Typ != newTyp {
            newStmt := dec.CopyNode()
            newStmt.(*plpgsqltree.Declaration).Typ = newTyp
            return newStmt, true
        }
    }
    return stmt, false
}

Nice suggestion. Done. If it comes up that walking the tree twice is a performance problem, we can always switch it back.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 60 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Could you add an exception block here and below?

Nice catch. Fixed a bug.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 133 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I doubt it's because of this PR, but do you know why the casts were changed to type annotations?

You mean like ::weekday? If we rename weekday, the casts will break if they're not rewritten, too.

DrewKimball

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rharding6373)

pkg/sql/show_create_clauses.go line 404 at r5 (raw file):

		ref := tree.UnresolvedObjectName{}
		ref.NumParts = 1
		ref.Parts[0] = t.Name()

Should we use fully qualified names here? There's a FQName method.

pkg/sql/sem/plpgsqltree/exception.go line 60 at r4 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Done.

Isn't it still wrong? stmt is the AST expression for the action, not the Exception itself. I think this would fail a test where multiple exception branches needed to change.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 133 at r4 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

You mean like ::weekday? If we rename weekday, the casts will break if they're not rewritten, too.

I mean for example we changed RETURN 'wednesday'::weekday; to RETURN 'wednesday':::test.public.weekday; - note the :: -> :::. I think this probably happens for SQL as well though, so not worth worrying about for too long.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 56 at r5 (raw file):

        CONTINUE;
      ELSIF nextval('seq') = 2 THEN
        SELECT v INTO i FROM nextval('seq') AS v(INT);

Can you add another ELSIF branch that needs replacement? Also, maybe also add a couple body statements to each branch. Something simple like SELECT nextval('seq') would be fine - we just want to make sure that all body statements are replaced correctly.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 63 at r5 (raw file):

  EXCEPTION
    WHEN division_by_zero THEN
      RAISE NOTICE USING MESSAGE = format('next val: %d',nextval('seq'));

Can you add multiple branches here that need replacement?

rharding6373

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

pkg/sql/show_create_clauses.go line 404 at r5 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Should we use fully qualified names here? There's a FQName method.

The qualification is in the parts. Added the schema and catalog.

pkg/sql/sem/plpgsqltree/exception.go line 60 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Isn't it still wrong? stmt is the AST expression for the action, not the Exception itself. I think this would fail a test where multiple exception branches needed to change.

Yes, you're right. I'll add the test coverage, too.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 133 at r4 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I mean for example we changed RETURN 'wednesday'::weekday; to RETURN 'wednesday':::test.public.weekday; - note the :: -> :::. I think this probably happens for SQL as well though, so not worth worrying about for too long.

Yeah, we do this elsewhere as well. This must be happening during FormatExprForDisplay. We're calling it with the tree.FmtParsable flag, though I don't know why or if it should be changing the casts.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 56 at r5 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Can you add another ELSIF branch that needs replacement? Also, maybe also add a couple body statements to each branch. Something simple like SELECT nextval('seq') would be fine - we just want to make sure that all body statements are replaced correctly.

Nice suggestions. Done.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 63 at r5 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Can you add multiple branches here that need replacement?

Done.

DrewKimball

Looks good. There are some failures where a body statement is omitted, though I can't see what might be causing it.

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rharding6373)

pkg/sql/show_create_clauses.go line 403 at r6 (raw file):

		}
		name := t.TypeMeta.Name
		ref, err := tree.NewUnresolvedObjectName(3, [3]string{name.Name, name.Schema, name.Catalog}, 0)

Are we guaranteed to have database and schema? If not, we could do:

tree.MakeTypeNameWithPrefix(ObjectNamePrefix{  
    CatalogName: tree.Name(name.Catalog),  
    SchemaName: tree.Name(name.Schema),  
    ExplicitCatalog: name.Catalog != "",  
    ExplicitSchema: name.ExplicitSchema,  
}, name.Name).ToUnresolvedObjectName()

DrewKimball

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rharding6373)

pkg/sql/alter_function.go line 95 at r7 (raw file):

	}

	log.Infof(context.Background(), "AlterFunction")

Is this left over from debugging?

pkg/sql/create_function.go line 458 at r7 (raw file):

	}

	if lang != catpb.Function_UNKNOWN_LANGUAGE && body != "" {

We should probably wring out cases where this happens, but it's fine by me if we do that in a different PR/issue. Hopefully it's a test-only problem.

pkg/sql/sem/plpgsqltree/statements.go line 1039 at r2 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

After thinking about this over the weekend, I decided to go with your suggestion and add unimplemented panics both here and in the visitor. They should trigger when someone adding these features adds their first logic test, so that should be enough to make sure they add both implementation and test coverage for these. Hopefully this is easier to review with ~half as much new code. Thanks for the suggestion!

Hmm, looks like we hit this code in the parser (at least the parser tests). So, we'll either have to rewrite the parser tests that use unsupported syntax or implement these statements after all. I think I'm in favor of the former, but I'm also fine with adding back the original implementations. Sorry for the back and forth.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 292 at r7 (raw file):

		}
	case *plpgsqltree.ForInt, *plpgsqltree.ForSelect, *plpgsqltree.ForCursor,
		*plpgsqltree.ForDynamic, *plpgsqltree.ForEachArray, *plpgsqltree.Exit,

Exit shouldn't be in this list.

rharding6373

Thanks for all the feedback! It seems like it's slowly converging.

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

pkg/sql/create_function.go line 458 at r7 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

We should probably wring out cases where this happens, but it's fine by me if we do that in a different PR/issue. Hopefully it's a test-only problem.

We need this (or something like this) because the alter function code path also uses this function and shouldn't have a language or a body option. Without this, we get the "body statement omitted" bug.

pkg/sql/show_create_clauses.go line 403 at r6 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Are we guaranteed to have database and schema? If not, we could do:

tree.MakeTypeNameWithPrefix(ObjectNamePrefix{  
    CatalogName: tree.Name(name.Catalog),  
    SchemaName: tree.Name(name.Schema),  
    ExplicitCatalog: name.Catalog != "",  
    ExplicitSchema: name.ExplicitSchema,  
}, name.Name).ToUnresolvedObjectName()

Not sure, since we do type resolution above. Your suggestion seems safer, so I implemented it.

pkg/sql/sem/plpgsqltree/statements.go line 1039 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Hmm, looks like we hit this code in the parser (at least the parser tests). So, we'll either have to rewrite the parser tests that use unsupported syntax or implement these statements after all. I think I'm in favor of the former, but I'm also fine with adding back the original implementations. Sorry for the back and forth.

How about something in the middle. I only added the implementation back for the features we are able to parse (presumably for telemetry reasons). We only have tests for CALL -- the FOR variants, non-vanilla RETURN, and PERFORM fail in the parser. That leaves dynamic execute and assert, which we don't support but presumably parse. I added those back for now, too, along with CALL.

Tangentially, this PR fixes what seems to be a bug/oversight with the telemetry visitor, since before this change we weren't walking the declarations or exceptions in a block, so now we get telemetry for those.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 292 at r7 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Exit shouldn't be in this list.

Not sure why I did that. Maybe because we don't support EXIT conditions yet, but yeah this is wrong.

pkg/sql/alter_function.go line 95 at r7 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Is this left over from debugging?

Yes, fixed.

DrewKimball

Thanks for making this happen!

Reviewed 5 of 6 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

mgartner

Looks great! I only had a few nitpicks.

Reviewed 4 of 10 files at r2, 1 of 34 files at r3, 2 of 7 files at r5, 1 of 3 files at r7, 2 of 6 files at r8, 4 of 4 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @rharding6373)

pkg/sql/create_view.go line 443 at r9 (raw file):

// replaceSeqNamesWithIDsLang prepares to walk the given query by defining the
// function used to replace sequence names with IDs, and parsing the
// queryStr into a statement.

nit: this description and the one for the function above don't make it super clear what the functions do, and what the expected return values are.

pkg/sql/create_view.go line 532 at r9 (raw file):

// serializeUserDefinedTypesLang will walk the given query and serialize any
// user defined types, so that renaming the type does not cause corruption.

nit: "and serialize any user defined types as IDs"

pkg/sql/create_view.go line 590 at r9 (raw file):

		return false, parsedExpr, nil
	}
	replaceTypeFunc := func(typ tree.ResolvableTypeReference) (newTyp tree.ResolvableTypeReference, err error) {

nit: Some explanation of the process here would be useful. Why do PLpgSQL bodies require two replacement steps, when SQL bodies only require one?

pkg/sql/show_create_clauses.go line 446 at r9 (raw file):

		v := utils.SQLStmtVisitor{Fn: replaceFunc}
		newStmt := plpgsqltree.Walk(&v, stmts)
		v2 := utils.TypeRefVisitor{Fn: replaceTypeFunc}

nit: ditto: an explanation for the two-step replacement would be helpful.

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 354 at r9 (raw file):

// TypeRefVisitor calls the given replace function on each type reference contained in the visited PLpgSQL
// statements. Note that this currently only includes `Declaration`, and SQL statements and expressions
// are not visited.

super nit: period after "Declaration", and the next part being its own sentence would be a little more clear.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 83 at r9 (raw file):


query TT
SHOW CREATE FUNCTION f_rewrite;

Should we rename the sequence and redo SHOW CREATE FUNCTION to ensure it propagated, or is that overkill? Same with the UDT test below.

CRDB rewrites sequence and UDT names as IDs in views and functions so that if the sequence or UDT is renamed the views and functions using them don't break. This PR adds support for this in PLpgSQL. Epic: None Fixes: cockroachdb#115627 Release note (sql change): Fixes a bug in PLpgSQL where altering the name of a sequence or UDT that was used in a PLpgSQL function or procedure could break them. This is only present in 23.2 alpha and beta releases.

rharding6373

TFTRs! Going to wait for a green test run before merging.

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

pkg/sql/sem/plpgsqltree/utils/plpg_visitor.go line 354 at r9 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

super nit: period after "Declaration", and the next part being its own sentence would be a little more clear.

Done.

pkg/ccl/logictestccl/testdata/logic_test/udf_rewrite line 83 at r9 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Should we rename the sequence and redo SHOW CREATE FUNCTION to ensure it propagated, or is that overkill? Same with the UDT test below.

Good suggestion. I had skipped this since we test this elsewhere though not as extensively. It doesn't hurt to add it, though, so I did.

rharding6373 · 2023-12-13T21:05:25Z

bors r+

craig · 2023-12-13T22:45:57Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-12-13T22:46:04Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 263a3d3 to blathers/backport-release-23.2-115809: 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 23.2.x failed. See errors above.

error creating merge commit from 263a3d3 to blathers/backport-release-23.2.0-rc-115809: 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 23.2.0-rc failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

rharding6373 requested a review from DrewKimball December 7, 2023 19:32

rharding6373 commented Dec 7, 2023

View reviewed changes

DrewKimball reviewed Dec 7, 2023

View reviewed changes

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch 2 times, most recently from 20609d8 to 015c973 Compare December 8, 2023 05:16

rharding6373 commented Dec 8, 2023

View reviewed changes

rharding6373 requested a review from mgartner December 8, 2023 05:20

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch from 015c973 to a08e4ca Compare December 8, 2023 20:40

DrewKimball reviewed Dec 9, 2023

View reviewed changes

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch from a08e4ca to c38cf2d Compare December 11, 2023 19:01

rharding6373 marked this pull request as ready for review December 11, 2023 19:01

rharding6373 requested review from a team as code owners December 11, 2023 19:01

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch from c38cf2d to a00f3fd Compare December 11, 2023 19:53

rharding6373 requested a review from DrewKimball December 11, 2023 19:54

rharding6373 commented Dec 11, 2023

View reviewed changes

DrewKimball reviewed Dec 11, 2023

View reviewed changes

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch from a00f3fd to 623265e Compare December 11, 2023 22:31

rharding6373 commented Dec 11, 2023

View reviewed changes

DrewKimball reviewed Dec 11, 2023

View reviewed changes

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch from 623265e to f1f1d8c Compare December 12, 2023 00:42

DrewKimball reviewed Dec 12, 2023

View reviewed changes

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch 2 times, most recently from cb918cb to 914f860 Compare December 12, 2023 05:13

rharding6373 commented Dec 12, 2023

View reviewed changes

DrewKimball approved these changes Dec 12, 2023

View reviewed changes

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch 2 times, most recently from 87c803c to e0f9c28 Compare December 12, 2023 18:30

mgartner requested a review from DrewKimball December 13, 2023 18:06

mgartner approved these changes Dec 13, 2023

View reviewed changes

mgartner added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-23.2.0-rc labels Dec 13, 2023

rharding6373 force-pushed the 20231205_plpgsql_seq_udt_id_rewrite_115627 branch from e0f9c28 to 263a3d3 Compare December 13, 2023 19:51

rharding6373 commented Dec 13, 2023

View reviewed changes

craig bot merged commit ebecdba into cockroachdb:master Dec 13, 2023
8 of 9 checks passed

This was referenced Dec 14, 2023

release-23.2: sql: support sequence and udt name rewriting in plpgsql #116419

Merged

release-23.2.0-rc: sql: support sequence and udt name rewriting in plpgsql #116420

Merged

stevendanna mentioned this pull request Dec 14, 2023

sql,backupccl: RESTORE with PLPGSQL function fails #116480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: support sequence and udt name rewriting in plpgsql #115809

sql: support sequence and udt name rewriting in plpgsql #115809

rharding6373 commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

cockroach-teamcity commented Dec 7, 2023

rharding6373 left a comment

rharding6373 left a comment

DrewKimball left a comment

rharding6373 left a comment

DrewKimball left a comment

rharding6373 left a comment

DrewKimball left a comment

rharding6373 left a comment

DrewKimball left a comment

DrewKimball left a comment

rharding6373 left a comment

DrewKimball left a comment

mgartner left a comment

rharding6373 left a comment

rharding6373 commented Dec 13, 2023

craig bot commented Dec 13, 2023

blathers-crl bot commented Dec 13, 2023

sql: support sequence and udt name rewriting in plpgsql #115809

sql: support sequence and udt name rewriting in plpgsql #115809

Conversation

rharding6373 commented Dec 7, 2023

blathers-crl bot commented Dec 7, 2023

cockroach-teamcity commented Dec 7, 2023

rharding6373 left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

rharding6373 commented Dec 13, 2023

craig bot commented Dec 13, 2023

blathers-crl bot commented Dec 13, 2023