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

Cursors with invalid names not handled properly in SQL/pgwire mix #84261

Closed
dvarrazzo opened this issue Jul 12, 2022 · 1 comment · Fixed by #85859
Closed

Cursors with invalid names not handled properly in SQL/pgwire mix #84261

dvarrazzo opened this issue Jul 12, 2022 · 1 comment · Fixed by #85859
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-blathers-triaged blathers was able to find an owner

Comments

@dvarrazzo
Copy link

dvarrazzo commented Jul 12, 2022

Describe the problem

If a cursor name is not a valid identifier (but can be used in double quotes), pgwire describe seems unable to find it.

To Reproduce

The following script fails with psycopg.errors.InvalidCursorName: unknown portal "1-2-3":

import os
import psycopg

with psycopg.connect(os.environ["DSN"]) as conn:
    with conn.cursor("1-2-3") as cur:
        cur.execute("select generate_series(1,3)")
        cur.fetchall() == [(1,), (2,), (3,)]

Handling this name at purely sql level has no problem:

defaultdb=> begin;
BEGIN

*defaultdb=> DECLARE "1-2-3" CURSOR FOR select generate_series(1, 3) as bar;
DECLARE CURSOR

*defaultdb=> fetch 1 from "1-2-3";
┌─────┐
│ bar │
├─────┤
│   1 │
└─────┘
(1 row)

Environment:

  • CockroachDB version 22.1.3

Jira issue: CRDB-17557
Epic CRDB-14049

@dvarrazzo dvarrazzo added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jul 12, 2022
@blathers-crl
Copy link

blathers-crl bot commented Jul 12, 2022

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

  • @cockroachdb/sql-experience (found keywords: import)

If we have not gotten back to your issue within a few business days, you can try the following:

  • Join our community slack channel and ask on #cockroachdb.
  • Try find someone from here if you know they worked closely on the area and CC them.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Jul 12, 2022
@rimadeodhar rimadeodhar added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Jul 12, 2022
rimadeodhar added a commit to rimadeodhar/cockroach that referenced this issue Aug 9, 2022
In CockroachDB and Postgres, it is possible to declare
cursors with invalid identifiers enclosed within double
quotes, for e.g. "1-2-3". However, when attempting to
look up the cursor during pgwire DESCRIBE step, it is
necessary to enclose the cursor name within double quotes
while looking up the cursor name within the CRDB code.
We don't currently do this, which causes the cursor look
up to fail during the DESCRIBE step.
In this PR, the cursor lookup step has been updated to
allow looking up a cursor name created by enclosing it in
double quotes.

Resolves cockroachdb#84261

Release note (bug fix): The pgwire DESCRIBE step no longer
fails with an error while attempting to look up cursors
declared by enclosing the cursor name within double quotes.
rimadeodhar added a commit to rimadeodhar/cockroach that referenced this issue Aug 12, 2022
In CockroachDB and Postgres, it is possible to declare
cursors with invalid identifiers enclosed within double
quotes, for e.g. "1-2-3". However, when attempting to
look up the cursor during pgwire DESCRIBE step, it is
necessary to enclose the cursor name within double quotes
while looking up the cursor name within the CRDB code.
We don't currently do this, which causes the cursor look
up to fail during the DESCRIBE step.
In this PR, the cursor lookup step has been updated to
allow looking up a cursor name created by enclosing it in
double quotes.

Resolves cockroachdb#84261

Release note (bug fix): The pgwire DESCRIBE step no longer
fails with an error while attempting to look up cursors
declared by enclosing the cursor name within double quotes.
rimadeodhar added a commit to rimadeodhar/cockroach that referenced this issue Sep 15, 2022
In CockroachDB and Postgres, it is possible to declare
cursors with special identifiers enclosed within double
quotes, for e.g. "1-2-3". Currently, we store the name
as an unescaped string which causes problems during the
pgwire DESCRIBE step for looking up the cursor. We should
be storing using the tree.Name datatype for the cursor name
while storing and looking up cursors. This PR updates the code
to start using tree.Name instead of raw strings for handling
cursor names. This fixes the issue where the pgwire DESCRIBE
step fails while attempting to look up cursors with names
containing special characters.

Resolves cockroachdb#84261

Release note (bug fix): The pgwire DESCRIBE step no longer
fails with an error while attempting to look up cursors
declared with names containing special characters.
rimadeodhar added a commit to rimadeodhar/cockroach that referenced this issue Sep 20, 2022
In CockroachDB and Postgres, it is possible to declare
cursors with special identifiers enclosed within double
quotes, for e.g. "1-2-3". Currently, we store the name
as an unescaped string which causes problems during the
pgwire DESCRIBE step for looking up the cursor. We should
be storing using the tree.Name datatype for the cursor name
while storing and looking up cursors. This PR updates the code
to start using tree.Name instead of raw strings for handling
cursor names. This fixes the issue where the pgwire DESCRIBE
step fails while attempting to look up cursors with names
containing special characters.

Resolves cockroachdb#84261

Release note (bug fix): The pgwire DESCRIBE step no longer
fails with an error while attempting to look up cursors
declared with names containing special characters.
craig bot pushed a commit that referenced this issue Sep 20, 2022
85859: pgwire: Add support for cursors with special characters r=knz,rafiss a=rimadeodhar

IIn CockroachDB and Postgres, it is possible to declare
cursors with special characters enclosed within double
quotes, for e.g. "1-2-3". Currently, we store the name
as an unescaped string which causes problems during the
pgwire DESCRIBE step for looking up the cursor. We should
be storing using the tree.Name datatype for the cursor name
while storing and looking up cursors. This PR updates the code
to start using tree.Name instead of raw strings for handling
cursor names. This fixes the issue where the pgwire DESCRIBE
step fails while attempting to look up cursors with names
containing special characters.

Resolves #84261

Release note (bug fix): The pgwire DESCRIBE step no longer
fails with an error while attempting to look up cursors
declared with names containing special characters.

86492: bulk: perform meta lookup on range cache miss during index backfill r=dt a=nvanbenschoten

Fixes #84290.

This commit addresses the sustained slowdown described in #84290 by replacing
the call in `SSTBatcher.flushIfNeeded` to `RangeCache.GetCached` with a call to
`RangeCache.Lookup`. The former method checks the cache but returns no range
descriptor if the cache currently has no overlapping key. This is possible if
the descriptor was recently evicted because it was stale. The later method
performs a meta lookup if the cache currently has no overlapping key, so it
should always return _some_ range descriptor.

There's a question of whether we should be logging a warning but proceeding if
this meta lookup fails. For now, I've decided not to change that behavior.

Release justification: None. Don't merge yet.

87885: kv: remove broken attempt to reject lease acquisitions on draining nodes r=nvanbenschoten a=nvanbenschoten

Related to #83261.

This commit removes "protection" that avoided lease acquisitions on draining nodes. This protection had already been effectively disabled by acc1ad1, which allowed Raft leaders to bypass the check. As the comment here (added in 5ffaa9e) explained, Raft followers are already unable to acquire the lease. If leaders bypass the check and follower (and candidates) don't need it, the check is useless, so we remove it.

The commit also removes `TestReplicaDrainLease`, which was supposed to test this behavior. We remove the test not because it started failing after the change, but because it did not. It must not have been testing anything real anymore after acc1ad1.

Release justification: low risk change related to release blocker.

Release note: None

88205: kvserver: (partially) deflake transfer-leases/drain-other-node r=irfansharif a=irfansharif

In #85629 we changed our lease transfer protocol to only ever transfer expiration-based leases, and have recipients later upgrade them to the more efficient epoch based ones. This was done to limit the effects of ill-advised lease transfers since the incoming leaseholder would need to recognize itself as such within a few seconds -- so we wanted this upgrade happen after having received the lease.

In #83261 however we noticed that the upgrade was not immediate -- we were waiting until the current lease's expiration was within its renewal duration -- 4.5s. When the lease was eventually renewed the upgrade did happen, but it was not immediate. We fix this here and remove the manual clock advancing the supporting test had that masked this issue. It now demonstrates that we're no longer relying on upgrades happen as part of the (slow) renewal process.

Release note: None

Co-authored-by: rimadeodhar <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: irfan sharif <[email protected]>
@craig craig bot closed this as completed in c0aa573 Sep 21, 2022
blathers-crl bot pushed a commit that referenced this issue Sep 21, 2022
In CockroachDB and Postgres, it is possible to declare
cursors with special identifiers enclosed within double
quotes, for e.g. "1-2-3". Currently, we store the name
as an unescaped string which causes problems during the
pgwire DESCRIBE step for looking up the cursor. We should
be storing using the tree.Name datatype for the cursor name
while storing and looking up cursors. This PR updates the code
to start using tree.Name instead of raw strings for handling
cursor names. This fixes the issue where the pgwire DESCRIBE
step fails while attempting to look up cursors with names
containing special characters.

Resolves #84261

Release note (bug fix): The pgwire DESCRIBE step no longer
fails with an error while attempting to look up cursors
declared with names containing special characters.
rimadeodhar added a commit to rimadeodhar/cockroach that referenced this issue Sep 22, 2022
In CockroachDB and Postgres, it is possible to declare
cursors with special identifiers enclosed within double
quotes, for e.g. "1-2-3". Currently, we store the name
as an unescaped string which causes problems during the
pgwire DESCRIBE step for looking up the cursor. We should
be storing using the tree.Name datatype for the cursor name
while storing and looking up cursors. This PR updates the code
to start using tree.Name instead of raw strings for handling
cursor names. This fixes the issue where the pgwire DESCRIBE
step fails while attempting to look up cursors with names
containing special characters.

Resolves cockroachdb#84261

Release note (bug fix): The pgwire DESCRIBE step no longer
fails with an error while attempting to look up cursors
declared with names containing special characters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-blathers-triaged blathers was able to find an owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants