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

sql,backupccl: RESTORE with PLPGSQL function fails #116480

Closed
stevendanna opened this issue Dec 14, 2023 · 5 comments · Fixed by #116650
Closed

sql,backupccl: RESTORE with PLPGSQL function fails #116480

stevendanna opened this issue Dec 14, 2023 · 5 comments · Fixed by #116650
Assignees
Labels
blocks-23.2.0-rc.1 branch-master Failures and bugs on the master branch. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-queries SQL Queries Team

Comments

@stevendanna
Copy link
Collaborator

stevendanna commented Dec 14, 2023

Describe the problem

On master (830dad6), we cannot seem to restore a database with a PLPGSQL function. I'm still investigating the root cause, but to reproduce:

CREATE SEQUENCE seq;
CREATE OR REPLACE FUNCTION foo() RETURNS INT AS
$$
  BEGIN
    RETURN nextval('seq');
  END
$$ LANGUAGE PLPGSQL
backup database defaultdb into 'userfile:///backup';
restore database defaultdb from latest in 'userfile:///backup' with new_db_name=new_defaultdb;

I get:

ERROR: at or near "return": syntax error
SQLSTATE: 42601
DETAIL: source SQL:
BEGIN
RETURN nextval('seq':::STRING)
^
HINT: try \h BEGIN

I haven't yet investigated further, but am marking as a GA-blocker until I can.

Jira issue: CRDB-34638

@stevendanna stevendanna added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-disaster-recovery GA-blocker labels Dec 14, 2023
Copy link

blathers-crl bot commented Dec 14, 2023

Hi @stevendanna, please add branch-* labels to identify which branch(es) this GA-blocker affects.

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

@blathers-crl blathers-crl bot added this to Triage in SQL Foundations Dec 14, 2023
Copy link

blathers-crl bot commented Dec 14, 2023

cc @cockroachdb/disaster-recovery

@stevendanna stevendanna added branch-master Failures and bugs on the master branch. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 labels Dec 14, 2023
@stevendanna
Copy link
Collaborator Author

I started looking into this because I was curious about whether the name->ID rewriting here #115809 required additional restore support since we rewrite descriptor IDs. However, I don' think this PR can be the cause of this failure because it hasn't merged to 23.2 yet but I can reproduce the above on beta3.

@stevendanna stevendanna added T-sql-queries SQL Queries Team and removed T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Dec 14, 2023
@stevendanna stevendanna removed this from Triage in SQL Foundations Dec 14, 2023
@stevendanna
Copy link
Collaborator Author

Some printf debugging says the error is coming from:

dbNameReplaced, err := rewriteFunctionBodyDBNames(fnDesc.FunctionBody, overrideDB)
if err != nil {
return err
}

@stevendanna
Copy link
Collaborator Author

stevendanna commented Dec 14, 2023

I think the problem is that the rewrite code for functions all call parser.Parse(). I'm not familiar with the APIs here, but looking at other code that deals with functions, it looks like we likely want a switch that also knows how to call plpgsql.Parser:

stmts, err := parser.Parse(fnBody)

@DrewKimball DrewKimball self-assigned this Dec 14, 2023
@dt dt removed this from Triage in Disaster Recovery Backlog Dec 15, 2023
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
This patch adds nil checks to the locations where `tree.SimpleVisit` or
`tree.SimpleStmtVisit` are called by `SQLStmtVisitor`. This is necessary
because those functions cannot handle nil arguments, and some fields of
PL/pgSQL AST structs can be unset.

Informs cockroachdb#116480

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
This commit includes the following fixes for the formatting methods of
the PL/pgSQL AST nodes:
1. Use `FmtCtx.FormatNode` instead of `node.Format`. This ensures that
   formatting flags are respected, including those used for redaction.
2. Similar to (1), use the correct `FmtCtx` methods for names and type
   references.
3. Print loop labels before and after the loop. This ensures that PL/pgSQL
   loops round trip through the pretty printer.
4. Print a semicolon at the end of formatting a `NULL` statement. This
   also ensures the pretty-printer round trips correctly.

Informs cockroachdb#116480

Release note (bug fix): Fixed the formatting for `PLpgSQL` routines,
which could prevent creating a routine with loop labels, and could
prevent some expressions from being redacted correctly. The bug only
existed in alpha and beta versions of 23.2.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
This commit factors out common logic between the SQL and PL/pgSQL parser
datadriven tests, so that the PL/pgSQL tests can now test that formatting
flags are handled correctly, and that the pretty printer correctly
round-trips. The improved testing coverage caught some bugs which have
been fixed in the previous commit.

Informs cockroachdb#116480

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
Previously, the redaction logic would return a syntax error upon attempting
to redact a PL/pgSQL UDF or stored procedure. This patch adds logic to switch
on the routine language, so that the correct parser is used during redaction.

Informs cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause a syntax error during
redaction of a PL/pgSQL routine. The bug existed only in alpha and beta
versions of the 23.2 release.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
This patch adds a switch on the routine language before replacements are
made in the body of a UDF or stored procedure. This avoids syntax errors
due to incorrectly using the SQL parser on a PL/pgSQL routine during
RESTORE.

Fixes cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause syntax errors when
attempting to RESTORE a database with PL/pgSQL UDFs or stored procedures.
This bug only affected alpha and beta versions of 23.2.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
Previously, the redaction logic would return a syntax error upon attempting
to redact a PL/pgSQL UDF or stored procedure. This patch adds logic to switch
on the routine language, so that the correct parser is used during redaction.

Informs cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause a syntax error during
redaction of a PL/pgSQL routine. The bug existed only in alpha and beta
versions of the 23.2 release.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
This patch adds a switch on the routine language before replacements are
made in the body of a UDF or stored procedure. This avoids syntax errors
due to incorrectly using the SQL parser on a PL/pgSQL routine during
RESTORE.

Fixes cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause syntax errors when
attempting to RESTORE a database with PL/pgSQL UDFs or stored procedures.
This bug only affected alpha and beta versions of 23.2.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
This commit factors out common logic between the SQL and PL/pgSQL parser
datadriven tests, so that the PL/pgSQL tests can now test that formatting
flags are handled correctly, and that the pretty printer correctly
round-trips. The improved testing coverage caught some bugs which have
been fixed in the previous commit.

Informs cockroachdb#116480

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 18, 2023
Previously, the redaction logic would return a syntax error upon attempting
to redact a PL/pgSQL UDF or stored procedure. This patch adds logic to switch
on the routine language, so that the correct parser is used during redaction.

Informs cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause a syntax error during
redaction of a PL/pgSQL routine. The bug existed only in alpha and beta
versions of the 23.2 release.
craig bot pushed a commit that referenced this issue Dec 18, 2023
116582: workflows: tag EngFlow build with PR link r=rail a=rickystewart

Epic: CRDB-8308
Release note: None

116650: sql,plpgsql: correctly handle formatting for PL/pgSQL routine redaction and rewrites r=mgartner a=DrewKimball

#### plpgsqltree: check for nil before calling Visit functions

This patch adds nil checks to the locations where `tree.SimpleVisit` or
`tree.SimpleStmtVisit` are called by `SQLStmtVisitor`. This is necessary
because those functions cannot handle nil arguments, and some fields of
PL/pgSQL AST structs can be unset.

Informs #116480

Release note: None

#### plpgsql: fix formatting methods

This commit includes the following fixes for the formatting methods of
the PL/pgSQL AST nodes:
1. Use `FmtCtx.FormatNode` instead of `node.Format`. This ensures that
   formatting flags are respected, including those used for redaction.
2. Similar to (1), use the correct `FmtCtx` methods for names and type
   references.
3. Print loop labels before and after the loop. This ensures that PL/pgSQL
   loops round trip through the pretty printer.
4. Print a semicolon at the end of formatting a `NULL` statement. This
   also ensures the pretty-printer round trips correctly.

Informs #116480

Release note (bug fix): Fixed the formatting for `PLpgSQL` routines,
which could prevent creating a routine with loop labels, and could
prevent some expressions from being redacted correctly. The bug only
existed in alpha and beta versions of 23.2.

#### sql,plpgsql: extend the SQL parser datadriven tests to handle PL/pgSQL

This commit factors out common logic between the SQL and PL/pgSQL parser
datadriven tests, so that the PL/pgSQL tests can now test that formatting
flags are handled correctly, and that the pretty printer correctly
round-trips. The improved testing coverage caught some bugs which have
been fixed in the previous commit.

Informs #116480

Release note: None

#### redact: handle PL/pgSQL routines during redaction

Previously, the redaction logic would return a syntax error upon attempting
to redact a PL/pgSQL UDF or stored procedure. This patch adds logic to switch
on the routine language, so that the correct parser is used during redaction.

Informs #116480

Release note (bug fix): Fixed a bug that would cause a syntax error during
redaction of a PL/pgSQL routine. The bug existed only in alpha and beta
versions of the 23.2 release.

#### backupccl: correctly handle PL/pgSQL routines during restore

This patch adds a switch on the routine language before replacements are
made in the body of a UDF or stored procedure. This avoids syntax errors
due to incorrectly using the SQL parser on a PL/pgSQL routine during
RESTORE.

Fixes #116480

Release note (bug fix): Fixed a bug that would cause syntax errors when
attempting to RESTORE a database with PL/pgSQL UDFs or stored procedures.
This bug only affected alpha and beta versions of 23.2.

116660: Revert "roachtest: remove or roachtests without post restore workload" r=dt a=msbutler

This reverts commit 1863417.

We realized that it would be nice compare download job speed with and without a workload.

Epic: none

Release note: none

Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Co-authored-by: Michael Butler <butler@cockroachlabs.com>
@craig craig bot closed this as completed in f554a1f Dec 18, 2023
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
This patch adds nil checks to the locations where `tree.SimpleVisit` or
`tree.SimpleStmtVisit` are called by `SQLStmtVisitor`. This is necessary
because those functions cannot handle nil arguments, and some fields of
PL/pgSQL AST structs can be unset.

Informs cockroachdb#116480

Release note: None
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
This commit includes the following fixes for the formatting methods of
the PL/pgSQL AST nodes:
1. Use `FmtCtx.FormatNode` instead of `node.Format`. This ensures that
   formatting flags are respected, including those used for redaction.
2. Similar to (1), use the correct `FmtCtx` methods for names and type
   references.
3. Print loop labels before and after the loop. This ensures that PL/pgSQL
   loops round trip through the pretty printer.
4. Print a semicolon at the end of formatting a `NULL` statement. This
   also ensures the pretty-printer round trips correctly.

Informs cockroachdb#116480

Release note (bug fix): Fixed the formatting for `PLpgSQL` routines,
which could prevent creating a routine with loop labels, and could
prevent some expressions from being redacted correctly. The bug only
existed in alpha and beta versions of 23.2.
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
This commit factors out common logic between the SQL and PL/pgSQL parser
datadriven tests, so that the PL/pgSQL tests can now test that formatting
flags are handled correctly, and that the pretty printer correctly
round-trips. The improved testing coverage caught some bugs which have
been fixed in the previous commit.

Informs cockroachdb#116480

Release note: None
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
Previously, the redaction logic would return a syntax error upon attempting
to redact a PL/pgSQL UDF or stored procedure. This patch adds logic to switch
on the routine language, so that the correct parser is used during redaction.

Informs cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause a syntax error during
redaction of a PL/pgSQL routine. The bug existed only in alpha and beta
versions of the 23.2 release.
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
This patch adds a switch on the routine language before replacements are
made in the body of a UDF or stored procedure. This avoids syntax errors
due to incorrectly using the SQL parser on a PL/pgSQL routine during
RESTORE.

Fixes cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause syntax errors when
attempting to RESTORE a database with PL/pgSQL UDFs or stored procedures.
This bug only affected alpha and beta versions of 23.2.
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
This patch adds nil checks to the locations where `tree.SimpleVisit` or
`tree.SimpleStmtVisit` are called by `SQLStmtVisitor`. This is necessary
because those functions cannot handle nil arguments, and some fields of
PL/pgSQL AST structs can be unset.

Informs cockroachdb#116480

Release note: None
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
This commit includes the following fixes for the formatting methods of
the PL/pgSQL AST nodes:
1. Use `FmtCtx.FormatNode` instead of `node.Format`. This ensures that
   formatting flags are respected, including those used for redaction.
2. Similar to (1), use the correct `FmtCtx` methods for names and type
   references.
3. Print loop labels before and after the loop. This ensures that PL/pgSQL
   loops round trip through the pretty printer.
4. Print a semicolon at the end of formatting a `NULL` statement. This
   also ensures the pretty-printer round trips correctly.

Informs cockroachdb#116480

Release note (bug fix): Fixed the formatting for `PLpgSQL` routines,
which could prevent creating a routine with loop labels, and could
prevent some expressions from being redacted correctly. The bug only
existed in alpha and beta versions of 23.2.
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
This commit factors out common logic between the SQL and PL/pgSQL parser
datadriven tests, so that the PL/pgSQL tests can now test that formatting
flags are handled correctly, and that the pretty printer correctly
round-trips. The improved testing coverage caught some bugs which have
been fixed in the previous commit.

Informs cockroachdb#116480

Release note: None
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
Previously, the redaction logic would return a syntax error upon attempting
to redact a PL/pgSQL UDF or stored procedure. This patch adds logic to switch
on the routine language, so that the correct parser is used during redaction.

Informs cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause a syntax error during
redaction of a PL/pgSQL routine. The bug existed only in alpha and beta
versions of the 23.2 release.
rytaft pushed a commit to rytaft/cockroach that referenced this issue Dec 18, 2023
This patch adds a switch on the routine language before replacements are
made in the body of a UDF or stored procedure. This avoids syntax errors
due to incorrectly using the SQL parser on a PL/pgSQL routine during
RESTORE.

Fixes cockroachdb#116480

Release note (bug fix): Fixed a bug that would cause syntax errors when
attempting to RESTORE a database with PL/pgSQL UDFs or stored procedures.
This bug only affected alpha and beta versions of 23.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocks-23.2.0-rc.1 branch-master Failures and bugs on the master branch. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-queries SQL Queries Team
Projects
Archived in project
4 participants