-
Notifications
You must be signed in to change notification settings - Fork 176
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
base: master
Are you sure you want to change the base?
Support for exclude table (-X) and exclude parent table (-Y) option #360
Conversation
…' into support-----exclude-table-option
I'm voting for not using short options for this feature, just the long ones. @za-arthur @Melkij What do you think? |
Personally, I don't use pg_repack without 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. |
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 --exclude-table --exclude-schema is a good option. I will work on that and create a pull soon. |
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. |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above exclude-parent-table
conflicts with table
and parent-table
:
https://github.com/reorg/pg_repack/pull/360/files#diff-a9d4d5b8968d339c50f089a6b531b1f36eedd27cdc008bdb26b70fb9fd0eee2fR382
There was a problem hiding this comment.
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.
I have removed short option names and provided long option names. --exclude-table Documentation is updated. |
Idea is to support pg_repack with below options
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