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

Support for exclude table (-X) and exclude parent table (-Y) option #360

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

chanukyasds
Copy link

@chanukyasds chanukyasds commented Jun 21, 2023

Idea is to support pg_repack with below options

  1. exclude-table (-X) : which skips a table from repacking
  2. exclude-parent-table (-Y): which skips a table and its children from repacking

These options has been added to the documentation.

please suggest the options if needed. (-X) and (-Y) are free currently.

Once PR is merged, I will be writing the regression tests.

Related issues
#18
#234

@chanukyasds chanukyasds changed the title Support for exclude table and exclude parent table option Support for exclude table (-X) and exclude parent table (-Y) option Jun 21, 2023
@andreasscherbaum
Copy link
Collaborator

I'm voting for not using short options for this feature, just the long ones.

@za-arthur @Melkij What do you think?

@Melkij
Copy link
Collaborator

Melkij commented Jul 17, 2023

Personally, I don't use pg_repack without -t at all, only with a specific table of a specific database. But there are no objections from me. These options do not overcomplicate the logic and it looks like they might be useful for someone. (maybe, for consistency, we also need --exclude-schema?)

Yes, I agree that it is better to add only long options. It would be better to have the -T option for --exclude-table (the pg_dump analogy would be very handy), but that option was already taken.

@chanukyasds
Copy link
Author

chanukyasds commented Jul 17, 2023

I agree -T is not available.

I think it's a better idea that we all can discuss and re-arrange options with best analogy.

Even I thought of below options for better understanding.

-t --table
-T --parent-table

--exclude-table
--exclude-parent-table

--exclude-schema is a good option. I will work on that and create a pull soon.

@za-arthur
Copy link
Collaborator

I agree that we probably want to add new short option names only if it is necessary and it is a good idea to add long names only for this PR.
We have limited number of short names and it is better to use them wise.

Copy link
Collaborator

@za-arthur za-arthur left a comment

Choose a reason for hiding this comment

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

Generally the PR looks good to me as well. I have couple comments below.
In the addition to those comments I'd suggest you to add new tests to test new options.

@@ -600,13 +628,27 @@ is_requested_relation_exists(char *errbuf, size_t errsize){
if (iparam < num_relations)
appendStringInfoChar(&sql, ',');
}
for (cell = exclude_table_list.head; cell; cell = cell->next)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic is probably unnecessary since exclude-table conflicts with table and parent-table:
https://github.com/reorg/pg_repack/pull/360/files#diff-a9d4d5b8968d339c50f089a6b531b1f36eedd27cdc008bdb26b70fb9fd0eee2fR377

Copy link
Author

@chanukyasds chanukyasds Jul 18, 2023

Choose a reason for hiding this comment

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

This logic helps pg_repack to find the exclude-table exists in the database or not.

I agree that exlude-table conflicts with table and parent-table options.

For example, consider below case.

pg_repack -p 5433 -d postgres -X test.table1

In above command user can skip a table from a database ,so we need to check the exclude-table (test.table1) existence.

some other examples:

pg_repack -p 5433 -d postgres -X test.table1 -X public.table_1

pg_repack -p 5433 -d postgres -X test.table1 -Y public.ptable2

for (cell = parent_table_list.head; cell; cell = cell->next)
{
appendStringInfo(&sql, "($%d, 'p')", iparam + 1);
params[iparam++] = cell->val;
if (iparam < num_relations)
appendStringInfoChar(&sql, ',');
}
for (cell = exclude_parent_table_list.head; cell; cell = cell->next)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

@chanukyasds chanukyasds Jul 18, 2023

Choose a reason for hiding this comment

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

Same as above.

This logic helps pg_repack to find the exclude-parent-table exists in the database or not.

For example consider below case.

pg_repack -p 5433 -d postgres -Y public.ptable2

In above command user can skip a parent table from a database, so we need to check the exclude-parent-table (public.ptable2) existence.

bin/pg_repack.c Outdated Show resolved Hide resolved
@chanukyasds
Copy link
Author

chanukyasds commented Jul 18, 2023

I have removed short option names and provided long option names.

--exclude-table
--exclude-parent-table

Documentation is updated.

@andreasscherbaum @Melkij @za-arthur

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants