Re: Handle infinite recursion in logical replication setup

On Mon, 5 Sept 2022 at 09:47, Peter Smith <smithpb2250@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.
> &lt;pub-names&gt;

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