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

Rename create_citus_local_table to citus_add_local_table_to_metadata #4573

Merged
merged 5 commits into from Jan 27, 2021

Conversation

onurctirtir
Copy link
Member

@onurctirtir onurctirtir commented Jan 25, 2021

DESCRIPTION: Drops support for create_citus_local_table in favor of citus_add_local_table_to_metadata

For simplicity in downgrade tests, didn't actually remove
create_citus_local_table udf.
Also made some small changes in error messages not
to mention create_citus_local_table udf anymore.

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #4573 (8151c4b) into master (1c7ee10) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4573   +/-   ##
=======================================
  Coverage   92.64%   92.64%           
=======================================
  Files         212      212           
  Lines       42479    42483    +4     
=======================================
+ Hits        39353    39359    +6     
+ Misses       3126     3124    -2     

@marcocitus
Copy link
Member

I think we can remove/rename the SQL-level create_citus_local_table, just not the C-level create_citus_local_table.

@onurctirtir
Copy link
Member Author

I think we can remove/rename the SQL-level create_citus_local_table, just not the C-level create_citus_local_table.

I tried to remove create_citus_citus_local_table dir, but then builds fail as 9.5-1 script still needs this directory.
So intead, I got rid of citus_add_clocal_table_to_metadata directory and moved 10.0-1 script to create_citus_citus_local_table dir. (41729cc)

Then I needed to rename create_citus_local_table udf in regression tests since now we don't have that in 10.0-1 (4be3db7)

What do you think @marcocitus ?

@onderkalaci
Copy link
Member

In addition to that, did not rename .c & .sql files and did not replace
create_citus_local_table with citus_add_local_table_to_metadata in
regression tests.

I think it'd better to rename this files as well?

@onderkalaci
Copy link
Member

I think it'd better to rename this files as well?

at least the c file

* of citus_add_local_table_to_metadata.
*/
Datum
create_citus_local_table(PG_FUNCTION_ARGS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have never exposed create_citus_local_table to the users. So, instead, can we simply give a ERROR saying that use citus_add_local_table_to_metadata ?

As you noted we had create_citus_local_table on the change log of 9.5, still we never showed in docs/blogs etc. And, with Citus 10, it should be OK to break some users.

Maybe add one line of extra DESCRIPTION: Drops support for create_citus_local_table in favor of citus_add_local_table_to_metadata or such

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This C function only exists to support testing 9.5->10.0 downgrade with a 10.0 binary (not a real scenario, just something our tests do). It does not matter much what we do here, since it should never be called in practice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but we have a test in those downgrade steps that first creates a citus local table in citus 9.5 and we error out if we try to downgrade back to 9.4, so I think we still need that udf in binary.
Or, I can remove that test too but not sure if this is a good option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd at least add a notice that create_citus_local_table is deprecated and use citus_add_local_table_to_metadata instead

Copy link
Member

@marcocitus marcocitus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me.

@@ -71,17 +71,18 @@ static void InsertMetadataForCitusLocalTable(Oid citusLocalTableId, uint64 shard
static void FinalizeCitusLocalTableCreation(Oid relationId);


PG_FUNCTION_INFO_V1(citus_add_local_table_to_metadata);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference to rename the file to citus_add_local_table_to_metadata.

@onurctirtir onurctirtir force-pushed the rename-create_citus_local_table branch from e3155d5 to 1ab6fcf Compare January 27, 2021 12:21
@onurctirtir onurctirtir force-pushed the rename-create_citus_local_table branch from dbcc5ba to 93a83d5 Compare January 27, 2021 12:52
@onurctirtir onurctirtir merged commit a18d428 into master Jan 27, 2021
@onurctirtir onurctirtir deleted the rename-create_citus_local_table branch January 27, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not expose citus local tables: Auto conversion from/to postgres to/from citus local tables for foreign keys
3 participants