On Mon, 5 Sept 2022 at 09:47, Peter Smith <smithpb2...@gmail.com> wrote: > > 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>
Modified
> ~
>
> 1b.
> The patch forgot to add the SQL "#" instruction as suggested by my
> previous comment (see [1] #3b)
Modified
> ~~~
>
> 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"
I have moved it to create subscription notes based on a recent comment
from Amit.
> ======
>
> 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, “)”);
> }
Modified
> ~~~
>
> 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)
Modified
> ~~
>
> 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.
Modified
> ~
>
> 5b. Variable 'first' is not necessary - it's same as (i == 0)
Modified
> ~
>
> 5c. The whole "SELECT" part and the ")" parts are more simply done at
> the caller (see the review comment #3)
Modified
> ~
>
> 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.
Modified
> ~
>
> 5e.
> "and" -> "AND"
Modified
> ~
>
> 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);
> + }
Modified
Thanks for the comments, the attached patch has the changes for the same.
Regards,
Vignesh
v46-0001-Check-and-log-a-warning-if-the-publisher-has-sub.patch
Description: Binary data
v46-0002-Document-the-steps-for-replication-between-prima.patch
Description: Binary data
