Here are my review comments for v45-0001:
======
1. doc/src/sgml/logical-replication.sgml
<para>
To find which tables might potentially include non-local origins (due to
other subscriptions created on the publisher) try this SQL query:
<programlisting>
SELECT DISTINCT N.nspname AS schemaname, C.relname AS tablename
FROM pg_publication P,
LATERAL pg_get_publication_tables(P.pubname) GPT
LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),
pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE C.oid = GPT.relid AND PS.srrelid IS NOT NULL AND
P.pubname IN (pub-names);
</programlisting></para>
1a.
To use "<pub-names>" with the <> then simply put meta characters in the SGML.
e.g.
<pub-names>
~
1b.
The patch forgot to add the SQL "#" instruction as suggested by my
previous comment (see [1] #3b)
~~~
2.
<sect1 id="preventing-inconsistencies-for-copy_data-origin">
<title>Preventing Data Inconsistencies for copy_data, origin=NONE</title>
The title is OK, but I think this should all be a <sect2> sub-section
of "31.2 Subscription"
======
3. src/backend/commands/subscriptioncmds.c - check_publications_origin
+ initStringInfo(&cmd);
+ appendStringInfoString(&cmd,
+ "SELECT DISTINCT P.pubname AS pubname\n"
+ "FROM pg_publication P,\n"
+ " LATERAL pg_get_publication_tables(P.pubname) GPT\n"
+ " LEFT JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
+ " pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
+ "WHERE C.oid = GPT.relid AND PS.srrelid IS NOT NULL AND P.pubname IN (");
+ get_publications_str(publications, &cmd, true);
+ appendStringInfoChar(&cmd, ')');
+ get_skip_tables_str(subrel_local_oids, subrel_count, &cmd);
(see from get_skip_tables_str)
+ appendStringInfoString(dest, " AND C.oid NOT IN (SELECT C.oid FROM
pg_class C JOIN pg_namespace N on N.oid = C.relnamespace where ");
IMO the way you are using get_skip_tables_str should be modified. I
will show by way of example below.
- "where" -> "WHERE"
- put the SELECT at the caller instead of inside the function
- handle the ")" at the caller
All this will also make the body of the 'get_skip_tables_str' function
much simpler (see the next review comments)
SUGGESTION
if (subrel_count > 0)
{
/* TODO - put some explanatory comment here about skipping the tables */
appendStringInfo(&cmd,
" AND C.oid NOT IN (\n"
"SELECT C.oid FROM pg_class C\n"
"JOIN pg_namespace N on N.oid = C.relnamespace WHERE ");
get_skip_tables_str(subrel_local_oids, subrel_count, &cmd);
appendStringInf(&cmd, “)”);
}
~~~
4. src/backend/commands/subscriptioncmds.c - get_skip_tables_str
+/*
+ * Add the table names that should be skipped.
+ */
This comment does not have enough detail to know really what the
function is for. Perhaps you only need to say that this is a helper
function for 'check_publications_origin' and then where it is called
you can give a comment (e.g. see my review comment #3)
~~
5. get_skip_tables_str (body)
5a. Variable 'count' is not a very good name; IMO just say 'i' for
index, and it can be declared C99 style.
~
5b. Variable 'first' is not necessary - it's same as (i == 0)
~
5c. The whole "SELECT" part and the ")" parts are more simply done at
the caller (see the review comment #3)
~
5d.
+ appendStringInfo(dest, "(C.relname = '%s' and N.nspname = '%s')",
+ tablename, schemaname);
It makes no difference but I thought it would feel more natural if the
SQL would compare the schema name *before* the table name.
~
5e.
"and" -> "AND"
~
Doing all 5a,b,c,d, and e means overall having a much simpler function body:
SUGGESTION
+ for (int i = 0; i < subrel_count; i++)
+ {
+ Oid relid = subrel_local_oids[i];
+ char *schemaname = get_namespace_name(get_rel_namespace(relid));
+ char *tablename = get_rel_name(relid);
+
+ if (i > 0)
+ appendStringInfoString(dest, " OR ");
+
+ appendStringInfo(dest, "(N.nspname = '%s' AND C.relname = '%s')",
+ schemaname, tablename);
+ }
------
[1]
https://www.postgresql.org/message-id/CAHut%2BPsku25%2BVjz7HiohWxc2WU07O_ZV4voFG%2BU7WzwKhUzpGQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
