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: show correct persistence information in pg_class table #56656

Closed
arulajmani opened this issue Nov 13, 2020 · 6 comments · Fixed by #56827
Closed

sql: show correct persistence information in pg_class table #56656

arulajmani opened this issue Nov 13, 2020 · 6 comments · Fixed by #56827
Labels
A-sql-pgcatalog A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@arulajmani
Copy link
Collaborator

Currently, regardless of persistence status (TEMP/UNLOGGED) pg_catalog.pg_class's relpersistence column returns p. Instead, this should return 't' for temp tables and 'u' for unlogged tables.

@arulajmani arulajmani added the A-sql-vtables Virtual tables - pg_catalog, information_schema etc label Nov 13, 2020
@blathers-crl
Copy link

blathers-crl bot commented Nov 13, 2020

Hi @arulajmani, please add a C-ategory label to your issue. Check out the label system docs.

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

@arulajmani arulajmani added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Nov 13, 2020
@rafiss rafiss added A-sql-pgcatalog A-sql-pgcompat Semantic compatibility with PostgreSQL labels Nov 13, 2020
@rafiss
Copy link
Collaborator

rafiss commented Nov 13, 2020

We don't actually support the functionality for unlogged tables so I think for now we will keep returning u for those. But we should handle temporary tables correctly

If an unlogged table is created now, this notice is sent

NOTICE: UNLOGGED TABLE will behave as a regular table in CockroachDB

@rafiss
Copy link
Collaborator

rafiss commented Nov 16, 2020

Watch out for this (and see if it can be fixed)

So 20.1 CREATE TABLE AS with temp tables does not seem to set the isTemporary bool on the table descriptor even if the temp option is specified on creation (https://github.com/cockroachdb/cockroach/blob/release-20.1/pkg/sql/parser/sql.y#L4423)

@arulajmani
Copy link
Collaborator Author

I think for now we will keep returning u for those.

We don't return u currently -- we return p, and we should switch to u.

Watch out for this (and see if it can be fixed)

There's a separate issue tracking this #56733 which I'm gonna be fixing. It is orthogonal to this issue though, as the comment above wrongly identifies the table descriptor state as the source of the problem.

@rafiss
Copy link
Collaborator

rafiss commented Nov 16, 2020

Oops my first comment had a mistake. I meant that it doesn't seem like we have an easy way of identifying UNLOGGED tables nor do we implement any UNLOGGED functionality right now, so we return p right now, and perhaps should keep returning p after this change.

@arulajmani
Copy link
Collaborator Author

Yeah, I dug into this a bit more and looks like we don't store UNLOGGED beyond the parser at the moment. I'm not sure if there's value in changing this so that we can return u during introspection, but unless we do decide to persist this information on disk I think we'll have to continue returning p for unlogged tables.

mnovelodou pushed a commit to mnovelodou/cockroach that referenced this issue Nov 18, 2020
Previously, the relpersistence column in pg_catalog.pg_class displayed 'p' for
all tables regardless of persistence status. This is incorrect for temporary
tables and unlogged tables, as they are supposed to have `t` and `u` values
respectively.

This patch fixes this behavior for temp tables, but leaves it unchanged for
unlogged tables (as we don't persist unlogged information past parsing).

Fixes cockroachdb#56656

Release note (sql change): the relpersistence column in pg_catalog.pg_class now
correctly displays `t` as the persistence status for temporary tables.
craig bot pushed a commit that referenced this issue Nov 23, 2020
56827: sql: Enabled correct pg_class.relPersistence for temporary tables r=arulajmani a=mnovelodou

Previously, the relpersistence column in pg_catalog.pg_class displayed 'p' for
all tables regardless of persistence status. This is incorrect for temporary
tables and unlogged tables, as they are supposed to have `t` and `u` values
respectively.

This patch fixes this behavior for temp tables, but leaves it unchanged for
unlogged tables (as we don't persist unlogged information past parsing).

Fixes #56656

Release note (sql change): the relpersistence column in pg_catalog.pg_class now
correctly displays `t` as the persistence status for temporary tables.

Co-authored-by: MiguelNovelo <[email protected]>
@craig craig bot closed this as completed in b1d1547 Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcatalog A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants