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

Cockroachdb misses support for: 'UNLISTEN *' 'DISCARD SEQUENCES' 'DISCARD TEMP' #83061

Closed
Meai opened this issue Jun 18, 2022 · 4 comments · Fixed by #86772
Closed

Cockroachdb misses support for: 'UNLISTEN *' 'DISCARD SEQUENCES' 'DISCARD TEMP' #83061

Meai opened this issue Jun 18, 2022 · 4 comments · Fixed by #86772
Assignees
Labels
A-tools-npgsql C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-starter Might be suitable for a starter project for new employees or team members. 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

@Meai
Copy link

Meai commented Jun 18, 2022

Describe the problem

Prepared statements with npqsql don't work.

To Reproduce

Try an sql insert with prepared parameters.

Expected behavior
No errors

Additional data / screenshots

The following sql features are used here:
https://github.com/npgsql/npgsql/blob/001c5ef35cf29ec9109e9a8d2ae54108a5d7ac9e/src/Npgsql/Internal/NpgsqlConnector.cs#L2061

Similar issue was already reported and fixed here but I don't know why that user didn't immediately have a problem again with the next sql statements in that method. #75435

  1. UNLISTEN *

  2. DISCARD SEQUENCES Npgsql.PostgresException (0x80004005): 0A000: at or near "sequences": syntax error: unimplemented: this syntax

  3. DISCARD TEMP Npgsql.PostgresException (0x80004005): 0A000: at or near "temp": syntax error: unimplemented: this syntax

Environment:

  • Npgsql version: current trunk commit npgsql/npgsql@0d8941a
  • Cockroachdb version: v22.1.1
  • Operating system: Opensuse Leap 15.3

Additional context
can't use cockroachdb unless I accept not having connections properly reset as described here: npgsql/npgsql#4515

Jira issue: CRDB-16832
Epic CRDB-14049

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

blathers-crl bot commented Jun 18, 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:

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 Jun 18, 2022
@roji
Copy link

roji commented Jun 18, 2022

For context... When a connection is returned to the pool, Npgsql resets it to prevent state leak. Usually this is done by sending a DISCARD ALL, and that seems to work with Cockroach.

However, if any prepared statements are in use, we want to persist them across pooled connections, to speed up short-lived connection scenarios (e.g. web app doing the same SQLs, but only once per pooled connection). DISCARD ALL deallocates all prepared statements, so we can't do that; instead we send the commands that DISCARD ALL is made up of, separately - except DEALLOCATE ALL (see the DISCARD ALL docs).

I suspect in Cockroach at least some of these are irrelevant, so you could simply ignore them. Let me know if anything is unclear or you need more info.

@roji
Copy link

roji commented Jun 18, 2022

And until this is fixed on the Cockroach side, this can be worked around by specifying No Reset On Close=true on the connection string - but that means state can leak across pooled connections.

@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Jun 21, 2022
@rafiss rafiss added the E-starter Might be suitable for a starter project for new employees or team members. label Jul 7, 2022
@rafiss
Copy link
Collaborator

rafiss commented Jul 12, 2022

Thanks for this report! We should be able to implement the two DISCARD statements.

We don't support LISTEN/NOTIFY, but we should be able to implement UNLISTEN by making at a no-op that shows a NOTICE that CockroachDB does not implement this feature.

@e-mbrown e-mbrown self-assigned this Aug 16, 2022
craig bot pushed a commit that referenced this issue Aug 25, 2022
85703: opt: add more tests for user-defined type dependency tracking r=mgartner a=mgartner

This commit updates the expression formatter to format user-defined type
dependencies, which were previously omitted. It also adds some
additional tests to ensure that user-defined type dependency tracking is
working as intended for `CREATE VIEW` and `CREATE FUNCTION`.

Release note: None

Release justification: This is a test-only change.


86230: sql: Support DISCARD SEQUENCES r=rafiss a=e-mbrown

Refs #83061

Release note (sql change): We now support DISCARD SEQUENCES, which discards
all sequence-related state such as currval/lastval information. DISCARD ALL
now also discards sequence-related state.

Release justification: Low risk, high benefit changes to existing functionality(DISCARD ALL)

86247: roachpb: cleanup speculative leases returned by NotLeaseHolderErrors r=nvanbenschoten a=arulajmani

Speculative leases are returned as part of NLHEs if the committed lease
is not known, but there is a guess as to who the leaseholder may be.
Previously, there were two ways to return these -- either by populating
just the LeaseHolder field on the NLHE or by populating the Lease field
with an unset sequence number. This patch unifies this behavior, and
going forward, we expect the latter to be the canonical form to
represent speculative leases. To that effect, the LeaseHolder field in
the NLHE proto is prefixed as "deprecated". We should be able to remove
the thing entirely in v23.1.

Release note: None
Release justification: low risk change.

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: e-mbrown <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
craig bot pushed a commit that referenced this issue Aug 31, 2022
86246: sql: Implement DISCARD TEMP r=rafiss a=e-mbrown

Refs #83061

Release note (sql change): We now support DISCARD TEMP/TEMPORARY, which drops all temporary tables created in the current session. The command does not drop temporary schemas.

Release note (bug fix): DISCARD ALL now deletes temporary tables

Release justification: Low risk, high benefit changes to existing functionality(DISCARD ALL)

86944: batcheval: fix incorrect StartTime in ExportResponse r=stevendanna a=adityamaru

Previously, ExportResponse for a revision history backup
was always setting the `StartTime` in the response to the
GCThreshold of the span being exported. This is correct for
a full backup where revision history stretches only as far back
as the GCThreshold, but is incorrect for incremental backups
where the StartTime should stretch as far back as the StartTime
of the ExportRequest.

Thankfully, this StartTime in ExportResponse is only used for
validating that the restore AOST is greater than the StartTime
of the full backup i.e. this field is inconsequential for incrementals.
Nonetheless, we should send back an accurate response.

Release note: None

Release justification: low risk bug fix

Co-authored-by: e-mbrown <[email protected]>
Co-authored-by: adityamaru <[email protected]>
@craig craig bot closed this as completed in 92102da Sep 1, 2022
craig bot pushed a commit that referenced this issue Jan 18, 2023
95398: sql/catalog/schemaexpr: fix bug when new column default has different… r=ajwerner a=ajwerner

… type

Since 22.2 we permit default expressions to contain types which are not exactly the same as the column type; it is valid to have an expression which can be cast to the column type in an assignment context. Generally, the optimizer handles inserting the assignment cast into the execution of the relevant mutations. Unfortunately, the cast was not present for backfills.

This PR detects the situation where a cast is needed and insert it directly into the plan of the backfill (or import).

Epic: None

Fixes: #93398

Release note (bug fix): Since 22.2, default expressions can have a type which differs from the type of the column so long as the expression's type can be cast in an assignment context. Unfortunately, this code had a bug when adding new columns. The code in the backfill logic was not sophisticated enough to know to add the cast; when such a default expression was added to a new column it would result in a panic during the backfill. This bug has now been fixed.

95414: sql: fix CLOSE ALL so it doesn't ignore the ALL flag r=jordanlewis a=rafiss

informs #83061

Release note (bug fix): Fixed a bug where CLOSE ALL would not respect the "ALL" flag and would instead attempt to close a cursor with no name.

95426: gossip: don't resolve addresses while holding mutex r=erikgrinaker a=erikgrinaker

This patch removes a DNS resolution call performed while holding the gossip mutex. This can lead to severe process stalls if the DNS lookup is not immediate, since we need to acquire gossip read locks in several performance critical code paths, including Raft processing. However, the DNS lookup was only done when validating a remote forwarding address, which presumably happens fairly rarely. Removing it should not cause any problems, since the address will necessarily be validated later when attempting to connect to it.

Epic: none
Release note (bug fix): Fixed a bug where a DNS lookup was performed during gossip remote forwarding while holding the gossip mutex. This could cause processing stalls if the DNS server was slow to respond.

Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tools-npgsql C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-starter Might be suitable for a starter project for new employees or team members. 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.

4 participants