Hi Shubham,
Some brief review comments about patch v3-0002.
======
Commit message
1.
This patch support the specification of both a WHERE clause (row filter) and a
column list in the table specification via pg_createsubscriber, and modify the
utility's name (for example, to a more descriptive or aligned name).
For eg:-
CREATE PUBLICATION pub FOR TABLE schema.table (col1, col2) WHERE
(predicate);
1a.
/support/supports/
~
1b.
"and modify the utility's name (for example, to a more descriptive or
aligned name)."
/modify/modifies/ but more importantly, what does this sentence mean?
======
GENERAL
2.
Should be some docs moved to this patch
~~~
3.
Missing test cases
======
src/bin/pg_basebackup/pg_createsubscriber.c
typedef struct TableSpec:
4.
char *dbname;
+ char *att_names;
+ char *row_filter;
char *pattern_regex;
4a.
FUNDAMENTAL DESIGN QUESTION
Why do we even want to distinguish these? IIUC, all you need to know
is "char *extra;" which can represent *anything* else the user may
tack onto the end of the table name. You don't even need to parse
it... e.g. You could let the "CREATE PUBLICATION" check the syntax.
~
4b.
Current patch is not even assigning these members (??)
~~~
5.
+char *
+pg_strcasestr(const char *haystack, const char *needle)
+{
+ if (!*needle)
+ return (char *) haystack;
+ for (; *haystack; haystack++)
+ {
+ const char *h = haystack;
+ const char *n = needle;
+
+ while (*h && *n && pg_tolower((unsigned char) *h) ==
pg_tolower((unsigned char) *n))
+ ++h, ++n;
+ if (!*n)
+ return (char *) haystack;
+ }
+ return NULL;
+}
+
Not needed.
~~~
create_publication:
6.
+ if (tbl->att_names && strlen(tbl->att_names) > 0)
+ appendPQExpBuffer(str, " ( %s )", tbl->att_names);
+
+ if (tbl->row_filter && strlen(tbl->row_filter) > 0)
+ appendPQExpBuffer(str, " WHERE %s", tbl->row_filter);
Overkill? I imagine this could all be replaced with something simple
without trying to distinguish between them.
if (tbl->extra)
appendPQExpBuffer(str, " %s", tbl->extra);
~~~
main:
7.
+ where_start = pg_strcasestr(copy_arg, " where ");
+ if (where_start != NULL)
+ {
+ *where_start = '\0';
+ where_start += 7;
+ }
+
+ paren_start = strchr(copy_arg, '(');
+ if (paren_start != NULL)
+ {
+ char *paren_end = strrchr(paren_start, ')');
+
+ if (!paren_end)
+ pg_fatal("unmatched '(' in --table argument \"%s\"", optarg);
+
+ *paren_start = '\0';
+ *paren_end = '\0';
+
+ paren_start++;
+ }
+
Overkill? IIUC, all you need to know is whether there's something
beyond the table name. Just looking for a space ' ' or a parenthesis
'(' delimiter could be enough, right?
e.g. Something like:
char *extra = strpbk(copy_arg, ' (');
if (extra)
ts->extra = extra + 1;
======
Kind Regards,
Peter Smith.
Fujitsu Australia
