Hi Nisha.
Some review comments for patch v6-0003.
======
Commit Message
1.
Extend AlterPublicationExceptTables() with the AP_SetObjects case,
which redefine the publication and replaces the entire EXCEPT list.
~
/redefine/redefines/
======
doc/src/sgml/ref/alter_publication.sgml
2.
-
-<phrase>and <replaceable
class="parameter">except_table_object</replaceable> is:</phrase>
-
- [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]
IIUC this is just removing some duplicate entry in the synopsis that
was not supposed to be there in the first place.
~~~
3.
The <literal>EXCEPT</literal> clause can be used with
- <literal>ADD TABLES IN SCHEMA</literal> to exclude specific tables from a
+ <literal>ADD TABLES IN SCHEMA</literal> and
+ <literal>SET TABLES IN SCHEMA</literal> to exclude specific tables from a
schema-level publication. <literal>EXCEPT</literal> is not supported with
<literal>DROP TABLES IN SCHEMA</literal>; instead, dropping a schema from
the publication automatically removes all of its associated
3a.
This whole description section seems arranged in a confusing way IMO.
Anyway, it is not all the fault of the current patch. But I don't
think it should be talking about "SET TABLES IN SCHEMA" here because
that was all mentioned already in the earlier "third variant"
paragraph.
~
3b.
That last sentence all about EXCEPT with DROP TABLES IN SCHEMA seems
redundant to me. It is not allowed by the synopsis, so there is no
possible confusion about it being supported. Isn't it better to just
say nothing?
~~~
EXCEPT
4.
<para>
Specifies tables to be excluded from a schema-level publication entry.
This clause may be used with <literal>ADD TABLES IN SCHEMA</literal>
- and not with <literal>DROP TABLES IN SCHEMA</literal>. Each named
- table must belong to the schema specified in the same
- <literal>TABLES IN SCHEMA</literal> clause. Table names may be
- schema-qualified or unqualified; unqualified names are implicitly
- qualified with the schema named in the same clause. See
- <xref linkend="sql-createpublication"/> for further details on the
+ and <literal>SET TABLES IN SCHEMA</literal>, and not with
+ <literal>DROP TABLES IN SCHEMA</literal>. Each named table must belong
+ to the schema specified in the same <literal>TABLES IN SCHEMA</literal>
+ clause. Table names may be schema-qualified or unqualified; unqualified
+ names are implicitly qualified with the schema named in the same clause.
+ See <xref linkend="sql-createpublication"/> for further details on the
semantics of <literal>EXCEPT</literal>.
</para>
4a.
IMO there is no reason to mention the "DROP TABLES IN SCHEMA" has no
EXCEPT. That is not possible just by looking at the synopsis, so there
is no ambiguity. Why say anything at all?
~
4b.
This description about EXCEPT is missing talking about FOR ALL TABLES
EXCEPT, but IIRC I already reported that in a previous review.
~~~
EXAMPLES
5.
+ Replace the schema list of <structname>sales_publication</structname> with
+ <structname>sales</structname>, excluding only
+ <structname>sales.drafts</structname> (any previously excluded tables for
+ that schema are replaced, and schemas previously in the publication are
+ removed):
BEFORE
(any previously excluded tables for that schema are replaced, and
schemas previously in the publication are removed):
SUGGESTION
Other than sales.drafts, any previously excluded tables for schema
sales are no longer excluded. Any schemas previously in the
sales_publication are removed.
~~~
6.
+<programlisting>
+ALTER PUBLICATION sales_publication SET TABLES IN SCHEMA sales EXCEPT
(TABLE sales.drafts);
+</programlisting>
Don't fully qualify that table in the SQL example.
======
src/backend/commands/publicationcmds.c
AlterPublicationExceptTables:
7.
- * Nothing to do if no EXCEPT entries.
+ * Nothing to do if no EXCEPT entries, except in SET: for that it is quite
+ * possible that the user has removed all exceptions, in which case we
+ * need to drop any existing ones.
Maybe reword this because it is a bit odd to have the word "except"
with the keyword "EXCEPT".
SUGGESTION
Nothing to do if there are no EXCEPT entries, unless handling the SET
command, because if the user has removed all exceptions we need to
drop any existing ones.
~~~
8.
+ {
+ List *oldexceptrelids = NIL;
+ List *newexceptrelids = NIL;
+ List *delrelids = NIL;
+ List *rels;
+ List *explicitrelids;
+ ListCell *lc;
+
+ rels = OpenTableList(exceptrelations);
+
+ /* Collect OIDs of the desired new except list. */
+ foreach(lc, rels)
There are multiple foreach() loops which can probably be simplified
all by using foreach_ptr(...) instead.
~~~
9.
+ /* Collect OIDs of the desired new except list. */
+ foreach(lc, rels)
/except/EXCEPT/
~~~
10.
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("table \"%s.%s\" cannot appear in both the table list and the
EXCEPT clause",
+ get_namespace_name(relns),
+ RelationGetRelationName(pri->relation)));
Use the new function to get a fully qualified name and just substitute
\"%s\" instead of \"%s.%s\".
~~~
11.
+ /*
+ * Get the current set of except entries. Only FOR ALL TABLES and
+ * schema-level publications can have except entries; for any other
+ * publication type oldexceptrelids stays NIL.
+ *
+ * Note: we check is_schema_publication() against the current catalog
+ * state (before AlterPublicationSchemas has run), so if the caller is
+ * doing SET TABLE t1 to convert a schema publication into a plain
+ * table publication, is_schema_publication() still returns true here.
+ * That is intentional: it lets us discover and clean up any stale
+ * except entries that belong to the old schema definition.
+ */
+ if (GetPublication(pubid)->alltables || is_schema_publication(pubid))
+ oldexceptrelids = GetExcludedPublicationTables(pubid,
+ PUBLICATION_PART_ROOT);
+
+ /* Build a list of old except entries not present in the new list. */
+ foreach(lc, oldexceptrelids)
+ {
+ Oid oldrelid = lfirst_oid(lc);
+
+ if (!list_member_oid(newexceptrelids, oldrelid))
+ delrelids = lappend_oid(delrelids, oldrelid);
+ }
+
+ /* Drop old except entries not present in the new list. */
+ foreach(lc, delrelids)
There are multiple comments here mentioning "except entries", but I
think they should say "EXCEPT entries".
~~~
PublicationDropSchemas:
12.
+ /*
+ * Collect prexcept rows for tables belonging to this schema before
+ * removing the schema entry. GetExcludedPublicationTables relies on
+ * is_schema_publication(), which scans pg_publication_namespace; if
+ * this is the last schema in the publication, performDeletion() below
+ * would remove that row and make is_schema_publication() return
+ * false, tripping the assertion.
+ */
What assertion?
~~~
13.
+ foreach(elc, exceptoids)
+ {
+ Oid relid = lfirst_oid(elc);
+ Oid proid;
Maybe this can be changed to be a foreach_oid(...) loop.
======
src/bin/psql/tab-complete.in.c
14.
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "TABLES",
"IN", "SCHEMA", MatchAny, "EXCEPT", "(", "TABLE", MatchAnyN) &&
ends_with(prev_wd, ','))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
Not sure if this also ought to be
'Query_for_list_of_tables_in_schema', instead of
'Query_for_list_of_tables'.
======
src/test/regress/sql/publication.sql
15.
+-- error: EXCEPT is not allowed with DROP
I think we should keep all the SET tests together. The DROP test seems
to be between other SET tests.
~~~
16.
Perhaps there should be some more tests -- eg. a test case to hit this
new error "table \"%s.%s\" cannot appear in both the table list and
the EXCEPT clause"
======
src/test/subscription/t/037_except.pl
17.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ INSERT INTO sch1.tab_published VALUES (7);
+ INSERT INTO sch1.tab_excluded VALUES (7);
+));
+$node_publisher->wait_for_catchup('sch_sub');
+
+$result =
+ $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM sch1.tab_excluded");
+is($result, qq(7),
+ 'ALTER ... SET TABLES IN SCHEMA EXCEPT: newly included table is replicated'
+);
+$result =
+ $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM sch1.tab_published");
+is($result, qq(6),
+ 'ALTER ... SET TABLES IN SCHEMA EXCEPT: now-excluded table is not replicated'
+);
Instead of having to keep track of the running totals IMO it might be
simpler if you just did "SELECT ... WHERE a = 7;" then the answer
would be just 1 or 0 rows.
~~~
18.
+# SET without EXCEPT: clears the except list; both tables are now published.
+# tab_published will be re-synced because REFRESH removed its entry when it was
+# excluded. Truncate the subscriber copy beforehand so the re-sync produces
+# a predictable count: publisher has 7 rows (6 original + INSERT(7)), so the
+# subscriber ends up with 7 after re-sync, then 8 after INSERT(8).
This is similar to my previous comment. There is lots of tricky
commentary here because you are trying to keep track of running totals
of rows.
I think most of this might not be needed if you changed the checks to
do ""SELECT ... WHERE a = 8;", so then you are just expecting row
count to be 1 for both tables.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
