Introduce a FDW option to mark reference tables #5189
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5189 +/- ##
==========================================
- Coverage 89.43% 89.41% -0.02%
==========================================
Files 225 225
Lines 51693 51719 +26
==========================================
+ Hits 46230 46245 +15
- Misses 5463 5474 +11
Continue to review full report at Codecov.
|
tsl/src/fdw/option.c
Outdated
| if (rel->rd_rel->relkind != RELKIND_RELATION) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_INVALID_TABLE_DEFINITION), | ||
| errmsg("table \"%s\" is not a relation. Only relations can be used as " |
There was a problem hiding this comment.
The message should be the other way around? "relation is not an ordinary table".
There was a problem hiding this comment.
@akuzm Thanks, it is changed.
tsl/src/fdw/relinfo.c
Outdated
| @@ -78,6 +78,13 @@ apply_fdw_and_server_options(TsFdwRelInfo *fpinfo) | |||
| option_extract_extension_list(defGetString(def), false)); | |||
| else if (strcmp(def->defname, "fetch_size") == 0) | |||
| fpinfo->fetch_size = strtol(defGetString(def), NULL, 10); | |||
| else if (strcmp(def->defname, "join_reference_tables") == 0) | |||
There was a problem hiding this comment.
In the name of the parameter, I'd draw attention to the fact that this is for replicated reference tables. E.g. replicated_reference_tables. Reference tables are not necessarily replicated, although we came to imply this in our terminology, so it might be confusing for the new users.
There was a problem hiding this comment.
A good point. I started a thread in Slack regarding the name.
There was a problem hiding this comment.
Renamed according to the discussion in Slack.
tsl/test/expected/dist_join-12.out
Outdated
| @@ -0,0 +1,173 @@ | |||
| -- This file and its contents are licensed under the Timescale License. | |||
There was a problem hiding this comment.
Would be good to have a more specific name for the test, like dist_ref_table_join.
There was a problem hiding this comment.
The files are renamed to dist_ref_table_join as suggested.
tsl/src/fdw/option.c
Outdated
| /* syntax error in name list */ | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_INVALID_PARAMETER_VALUE), | ||
| errmsg("parameter \"%s\" must be a list of reference table names", |
There was a problem hiding this comment.
| errmsg("parameter \"%s\" must be a list of reference table names", | |
| errmsg("parameter \"%s\" must be a comma-separated list of reference table names", |
There was a problem hiding this comment.
@erimatnor Thanks for the review. The error messages are changed in the current version of the PR.
tsl/src/fdw/option.c
Outdated
| { | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_INVALID_TABLE_DEFINITION), | ||
| errmsg("table \"%s\" does not exist.", tablename))); |
There was a problem hiding this comment.
| errmsg("table \"%s\" does not exist.", tablename))); | |
| errmsg("table \"%s\" does not exist", tablename))); |
tsl/src/fdw/option.c
Outdated
| ereport(ERROR, | ||
| (errcode(ERRCODE_INVALID_TABLE_DEFINITION), | ||
| errmsg("table \"%s\" is not a relation. Only relations can be used as " | ||
| "reference tables.", |
There was a problem hiding this comment.
| "reference tables.", | |
| "reference tables", |
jnidzwetzki
force-pushed
the
mn_join_fdw_option
branch
2 times, most recently
from
0a5057c
to
2f4f716
Compare
With this patch, the ability to mark reference tables (tables that exist on all data nodes of a multi-node installation) via an FDW option has been added.
jnidzwetzki
force-pushed
the
mn_join_fdw_option
branch
from
2f4f716
to
9958d67
Compare
With this patch, the ability to mark reference tables (tables that exist on all data nodes of a multi-node installation) via an FDW option has been added.
Note: This patch is part of a series of patches that allows join pushdowns in the multi-node code. This commit also introduces the regression test files for the join pushdowns. The test output is currently the same for all PG versions. However, this will change when the test cases for the join pushdown are added.