Re: Handle infinite recursion in logical replication setup

On Sat, Jul 2, 2022 at 12:12 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jun 30, 2022 at 9:40 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Thu, Jun 30, 2022 at 9:17 AM shiy(dot)fnst(at)fujitsu(dot)com
> > <shiy(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
>
> The first patch that adds a test case for existing functionality looks
> good to me and I'll push that early next week (by Tuesday) unless
> there are more comments on it.
>
> Few minor comments on 0002
> ========================
> 1.
> + /* FIXME: 150000 should be changed to 160000 later for PG16. */
> + if (options->proto.logical.origin &&
> + PQserverVersion(conn->streamConn) >= 150000)
> + appendStringInfo(&cmd, ", origin '%s'",
> + options->proto.logical.origin);
>
> ...
> ...
> + /* FIXME: 150000 should be changed to 160000 later for PG16. */
> + if (fout->remoteVersion >= 150000)
> + appendPQExpBufferStr(query, " s.suborigin\n");
> + else
> + appendPQExpBuffer(query, " '%s' AS suborigin\n", LOGICALREP_ORIGIN_ANY);
> ...
> ...
> + /* FIXME: 150000 should be changed to 160000 later for PG16 */
> + if (pset.sversion >= 150000)
> + appendPQExpBuffer(&buf,
> + ", suborigin AS \"%s\"\n",
> + gettext_noop("Origin"));
>
> All these should now change to 16.

Modified

> 2.
> /* ALTER SUBSCRIPTION <name> SET ( */
> else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
> TailMatches("SET", "("))
> - COMPLETE_WITH("binary", "slot_name", "streaming",
> "synchronous_commit", "disable_on_error");
> + COMPLETE_WITH("binary", "origin", "slot_name", "streaming",
> "synchronous_commit", "disable_on_error");
> /* ALTER SUBSCRIPTION <name> SKIP ( */
> else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
> TailMatches("SKIP", "("))
> COMPLETE_WITH("lsn");
> @@ -3152,7 +3152,7 @@ psql_completion(const char *text, int start, int end)
> /* Complete "CREATE SUBSCRIPTION <name> ... WITH ( <opt>" */
> else if (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
> COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
> - "enabled", "slot_name", "streaming",
> + "enabled", "origin", "slot_name", "streaming",
> "synchronous_commit", "two_phase", "disable_on_error");
>
> Why do you choose to add a new option in-between other parameters
> instead of at the end which we normally do? The one possible reason I
> can think of is that all the parameters at the end are boolean so you
> want to add this before those but then why before slot_name, and again
> I don't see such a rule being followed for other parameters.

I was not sure if it should be maintained in alphabetical order,
anyway since the last option "disable_on_error" is at the end, I have
changed it to the end.

Thanks for the comments, the attached v27 patch has the changes for the same.

Regards,
Vignesh