Re: Add support for specifying tables in pg_createsubscriber.

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