Re: Skipping schema changes in publication

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