Hi Shlok.
Here are some review comments for the patch v3-0001.
(not yet reviewed the test code)
======
doc/src/sgml/ref/alter_publication.sgml
synopsis:
1.
- [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]
+ TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ]
This is still not correct because it is missing the ellipsis that is
needed within 'except_table_object'. e.g, the currently documented
synopsis would not permit FOR ALL TABLE EXCEPT (TABLE t1,t2,t3);
You might describe that using one of these ways:
i)
TABLE { [ ONLY ] table_name [ * ] } [, ...]
ii)
TABLE [ ONLY ] table_name [ * ] [, ...]
iii)
TABLE table [,...]
and table is:
[ ONLY ] table_name [ * ]
======
doc/src/sgml/ref/create_publication.sgml
synopsis:
2.
Same synopsis problem as described in the above review comment. It is
missing the ellipsis that is needed within 'except_table_object'.
~~~
EXCEPT:
3.
<varlistentry id="sql-createpublication-params-for-except-table">
- <term><literal>EXCEPT TABLE</literal></term>
+ <term><literal>EXCEPT</literal></term>
<listitem>
The 'EXCEPT' clause is not really at the same level as all the other
ones, because it can only be applied to the FOR ALL TABLES. So, I felt
maybe 'EXCEPT' might make more sense as a sublist entry under the FOR
ALL TABLES clause.
Moving this would also eliminate some of the current repetition:
e.g. "Tables listed in EXCEPT clause are excluded from the publication."
e.g. "This clause specifies a list of tables to be excluded from the
publication."
======
src/backend/catalog/pg_publication.c
GetAllPublicationRelations:
4.
- * in EXCEPT TABLE clause.
+ * in EXCEPT clause.
/in EXCEPT clause/in the EXCEPT clause/
======
src/backend/parser/gram.y
5.
static void preprocess_pubobj_list(List *pubobjspec_list,
core_yyscan_t yyscanner);
+static void preprocess_except_pubobj_list(List *pubexceptobjspec_list,
+ core_yyscan_t yyscanner);
The new function and the parameter names do not match as they do in
the existing function. e.g. Should this new function instead be called
'preprocess_pubexceptobj_list'?
~~~
preprocess_except_pubobj_list:
6.
+ ListCell *cell;
+ PublicationObjSpec *pubobj;
Why is this function referring to PublicationObjSpec instead of
PublicationExceptObjSpec. Is it correct?
~~~
7.
+ if (pubobj->pubobjtype == PUBLICATIONOBJ_CONTINUATION)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid publication object list"),
+ errdetail("TABLE must be specified before a standalone table."),
+ parser_errposition(pubobj->location));
If this was intended to mimic code from the existing function, then I
felt it should say "standalone table name." instead of "standalone
table.".
======
src/bin/pg_dump/pg_dump.c
8.
if (++n_except == 1)
- appendPQExpBufferStr(query, " EXCEPT TABLE (");
+ appendPQExpBufferStr(query, " EXCEPT (TABLE ");
else
- appendPQExpBufferStr(query, ", ");
+ appendPQExpBufferStr(query, ", TABLE ");
You don't need the TABLE keyword in 2 places like that.
SUGGESTION
if (++n_except == 1)
appendPQExpBufferStr(query, " EXCEPT (");
else
appendPQExpBufferStr(query, ", ");
appendPQExpBuffer(query, "TABLE ONLY %s", fmtQualifiedDumpable(tbinfo));
======
src/bin/pg_dump/t/002_pg_dump.pl
9.
'CREATE PUBLICATION pub10' => {
create_order => 92,
create_sql =>
- 'CREATE PUBLICATION pub10 FOR ALL TABLES EXCEPT TABLE
(dump_test.test_inheritance_parent);',
+ 'CREATE PUBLICATION pub10 FOR ALL TABLES EXCEPT (TABLE
dump_test.test_inheritance_parent);',
regexp => qr/^
- \QCREATE PUBLICATION pub10 FOR ALL TABLES EXCEPT TABLE (ONLY
dump_test.test_inheritance_parent, ONLY
dump_test.test_inheritance_child) WITH (publish = 'insert, update,
delete, truncate');\E
+ \QCREATE PUBLICATION pub10 FOR ALL TABLES EXCEPT (TABLE ONLY
dump_test.test_inheritance_parent, TABLE ONLY
dump_test.test_inheritance_child) WITH (publish = 'insert, update,
delete, truncate');\E
/xm,
like => { %full_runs, section_post_data => 1, },
(Maybe this question is unrelated to the current patch.)
Why is the expected result explicitly including ONLY children like
that instead of saying "EXCEPT (TABLE
dump_test.test_inheritance_parent *)"?
======
src/bin/psql/tab-complete.in.c
10.
- else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "EXCEPT", "TABLE", "(", MatchAnyN) && ends_with(prev_wd,
','))
+ else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "EXCEPT", "(", "TABLE", MatchAnyN) && ends_with(prev_wd,
','))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
- else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "EXCEPT", "TABLE", "(", MatchAnyN) && !ends_with(prev_wd,
','))
+ else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
"TABLES", "EXCEPT", "(", "TABLE", MatchAnyN) && !ends_with(prev_wd,
','))
COMPLETE_WITH(")");
The CREATE PUBLICATION seems to have tab-completion logic for the ','
separator and closing ')'. But I did not see any similar
tab-completion logic for the EXCEPT clause in ALTER PUBLICATION. Is
this maybe another issue independent of this patch?
======
src/test/regress/expected/publication.out
src/test/regress/sql/publication.sql
src/test/subscription/t/037_except.pl
(I will review these later)
======
GENERAL
11.
The file src/backend/commands/publicationcmds.c still refers to
"(EXCEPT TABLES)".
======
Kind Regards,
Peter Smith.
Fujitsu Australia
